Skip to content

Conversation

ntfshard
Copy link
Contributor

@ntfshard ntfshard commented Oct 20, 2025

🦟 Bug fix

Fixes #

Summary

Adding model with Entity Tree gui plugin, or in sdf w/o material won't creating a material entity in ECM. Object will looks flat and it's not possible to change material properties in gui. Populating default material will allow to configure it in gui.

Behaviour changing: before color of model was white and after this change it will be black. According to SDF specification http://sdformat.org/spec?elem=material&ver=1.9 it's correct behaviour. So technically it's a fix

(This is a port of changes from #3009 to main branch; and screenshots are available in initial PR)

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development Oct 21, 2025
@arjo129
Copy link
Contributor

arjo129 commented Oct 21, 2025

I'm hesitant to back-port this. I know @azeey suggested back-porting, but it would mean some end users get a bit of a shock when they wake up to find their simulations change color. Perhaps the less jarring fix would be to make the default white in previous gz-sim versions and then keep things black on the main branch.

@ntfshard
Copy link
Contributor Author

I'm hesitant to back-port this. I know @azeey suggested back-porting, but it would mean some end users get a bit of a shock when they wake up to find their simulations change color. Perhaps the less jarring fix would be to make the default white in previous gz-sim versions and then keep things black on the main branch.

I guess "their simulations changed color" is a very big generalization in this context. I guess there are no users who are using models w/o materials due to you can't see any edges and faces of a model. It's just a uniformly colored silhouette of the model (first screenshot in initial pull request). It looks strange and not convenient (in context of 3d graphics).

Keeping different behaviour will just increase complexity of codebase support imo. More merge conflicts

@scpeters
Copy link
Member

I think there are two concerns that this pull request addresses:

  1. "it's not possible to change material properties in gui. Populating default material will allow to configure it in gui."
  2. The default material properties don't match the SDFormat specification (white instead of black).

I would recommend separating these two concerns into separate pull requests:

  1. Add a material (white by default) to ECM so that it can be configured through the GUI.
  2. Change the default material properties from white to something else.

I think it would not be controversial to merge and backport the bugfix for the first concern. The second concern is more complex, but I would rather have that discussion in a way that doesn't block the bugfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants