Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use scene name for texture name in assimp loader #563

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Dec 13, 2023

🦟 Bug fix

Summary

When importing assimp scenes, the name of the generated texture used the name of the root node as a prefix.
This worked well in simple GLB assets where there is only one node and the name would be its name but the moment a user was to import a scene with more than one node assimp would automatically create a new node and hardcode its name to ROOT as shown here.

This creates issues when running gz with ogre2 since we use the texture name to share textures between different assets, so different files would result in textures called ROOT_[...] and would cause the wrong texture to be displayed.

The fix is to use the scene name instead of the root node name (which was probably the right implementation in the first place).

Update: Found out why this wasn't the solution in the first place, CI fails on Focal since the field didn't exist in the assimp version released on 20.04:

  /github/workspace/graphics/src/AssimpLoader.cc:587:27: error: 'const struct aiScene' has no member named 'mName'
    587 |   return ToString(_scene->mName) + "_" + ToString(_mat->GetName()) +
        |                           ^~~~~
  make[2]: *** [graphics/src/CMakeFiles/gz-common5-graphics.dir/build.make:76: graphics/src/CMakeFiles/gz-common5-graphics.dir/AssimpLoader.cc.o] Error 1

Closing to retarget main.

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

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

@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Dec 13, 2023
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova force-pushed the luca/assimp_material_scene_name branch from f8376c7 to 4e5bba5 Compare December 13, 2023 02:28
@luca-della-vedova luca-della-vedova changed the base branch from gz-common5 to main December 13, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant