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

Introduces GZ_MESH_FORCE_ASSIMP flag #403

Merged

Conversation

onurtore
Copy link
Contributor

@onurtore onurtore commented Jul 27, 2022

These flags utilized to use assimp for mesh loading process. If useAssimp flag is true then STL, GLTF and FBX meshes are loaded using assimp library, otherwise old loaders will be used. forceAssimp flags used to load all supported mesh formats using assimp.

Onur Berk Tore added 5 commits July 25, 2022 23:12
Signed-off-by: Onur Berk Tore <[email protected]>
Signed-off-by: Onur Berk Tore <[email protected]>
Signed-off-by: Onur Berk Tore <[email protected]>
These flags utilized to use assimp for mesh loading
process. If useAssimp flag is true then STL, GLTF
and FBX meshes are loaded using assimp library,
otherwise old loaders will be used. forceAssimp
flags used to load all supported mesh formats
using assimp.

Signed-off-by: Onur Berk Tore <[email protected]>
@onurtore onurtore requested a review from mjcarroll as a code owner July 27, 2022 17:05
@onurtore onurtore changed the title Otore19/assimp flag env Introduces useAssimp and forceAssimp flags Jul 27, 2022
@onurtore
Copy link
Contributor Author

@chapulina,
What do you think?

Onur Berk Tore added 3 commits July 27, 2022 20:08
Signed-off-by: Onur Berk Tore <[email protected]>
Signed-off-by: Onur Berk Tore <[email protected]>
Signed-off-by: Onur Berk Tore <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you document the new environment variables? Maybe on the MeshManager's header?

graphics/src/MeshManager.cc Outdated Show resolved Hide resolved
Onur Berk Tore added 2 commits July 28, 2022 18:16
MeshManager only uses forceAssimp flag.

Signed-off-by: Onur Berk Tore <[email protected]>
@onurtore onurtore requested a review from chapulina July 28, 2022 15:27
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! Since we're not checking extensions when forcing assimp, it would be nice to add a test where GZ_MESH_FORCE_ASSIMP=true and we try to load a mesh format that isn't supported by assimp (i.e. mesh.invalid), to make sure that doesn't cause a crash.

Signed-off-by: Onur Berk Tore <[email protected]>
@chapulina chapulina added the 🌱 garden Ignition Garden label Jul 28, 2022
@chapulina chapulina changed the title Introduces useAssimp and forceAssimp flags Introduces GZ_MESH_FORCE_ASSIMP flag Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #403 (ee6badb) into luca/assimp_sandbox (4484e5e) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@                   Coverage Diff                   @@
##           luca/assimp_sandbox     #403      +/-   ##
=======================================================
- Coverage                80.35%   80.34%   -0.01%     
=======================================================
  Files                       85       85              
  Lines                     9933     9944      +11     
=======================================================
+ Hits                      7982     7990       +8     
- Misses                    1951     1954       +3     
Impacted Files Coverage Δ
graphics/src/MeshManager.cc 91.82% <80.00%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4484e5e...ee6badb. Read the comment docs.

@luca-della-vedova luca-della-vedova merged commit afc47c8 into gazebosim:luca/assimp_sandbox Jul 29, 2022
@onurtore
Copy link
Contributor Author

onurtore commented Aug 3, 2022

Looks great, thanks! Since we're not checking extensions when forcing assimp, it would be nice to add a test where GZ_MESH_FORCE_ASSIMP=true and we try to load a mesh format that isn't supported by assimp (i.e. mesh.invalid), to make sure that doesn't cause a crash.

Hi @chapulina,
I think IsValidFilename function in MeshManager checks regardless of the mesh loader, and assimp loader covers the previous formats as well, so currently there is no unsupported format for the assimp loader.

On the other hand, I am very open to ideas about possible test cases, so if you have anything in mind, I would love to implement it.

@chapulina
Copy link
Contributor

I added a simple test in a1c0b9f as part of #393, that loads a non-supported extension and verifies that a nullptr is returned. That's all I had in mind 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants