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

Assimp Loader with Recursion #355

Closed
wants to merge 9 commits into from
Closed

Assimp Loader with Recursion #355

wants to merge 9 commits into from

Conversation

onurtore
Copy link
Contributor

@onurtore onurtore commented May 17, 2022

Hi,
I believe you can use some part of this code to further improve this branch. The existing structure lacks recursive loading, but the codes I shared can be utilized to implement that future. (Just look AssimpLoader::MeshFromAssimpScene and AssimpLoader::buildMesh). This structure is similar to how Rviz handles the Assimp (Which is taken from Gazebo :) .)

I was planning to work on this and further compare assimp on different metrics (size, speed)

@onurtore onurtore requested a review from mjcarroll as a code owner May 17, 2022 20:14
@onurtore
Copy link
Contributor Author

@luca-della-vedova

@luca-della-vedova
Copy link
Member

Hi @onurtore ! This is super helpful, indeed the meshes should be built in a recursive way and not iterative as I was doing in a first quick draft, the limitations became evident when parsing scenes with complex hierarchies where each transform is derived from the parent transform (i.e. bones for animations).
I think this is a very large and complex task and help is definitely welcome, there are a lot of TODOs scattered around (and to be honest I put "sandbox" in the name because the code is very 🔥 right now, this is still in a feasibility study state for me, where I want to see if we can get all the basic features out of assimp before refactoring and polishing the code).
I was hoping assimp could be a full replacement for all the loaders and was hoping to prove that "it works" by merging all the unit tests in our loaders into a single AssimpLoader_TEST.cc file and making sure they work, I started this and documented the first few items not working so they can be a good target for fixes / documentation where needed (i.e. shininess, number of materials on top of my head).
I was also hoping that we could either remove hardcoded supported mesh formats or greatly increase their number since assimp supports a whole variety of them.

@onurtore
Copy link
Contributor Author

onurtore commented May 18, 2022

Hi @luca-della-vedova,
I think we should create a tentative guideline for the work. Whether Gazebo should use Assimp or not is a question by itself. Even if everything works with Assimp it might be bottleneck in some cases. (Speed, size, ...) So a basic tentative assignment in my mind was something like this:

[-] Converting your code base to allow recursive loading.
[-] Implementing basic mesh loading capabilities.
[-] Comparing assimp with different methods on different metrics, such as: Size, Speed, Dependencies, Code Complexity, Code Quality
-In this step, measurements needs to be done with the gazebo profiler.
- I created a simple report for this step, you can access it from here.
[-] Utilizing assimp or different method using the result of previous step.

@luca-della-vedova
Copy link
Member

Thanks for the PR! There was a lot of duplicated logic here but I followed your advice and made mesh / skeleton creation recursive to make sure the transforms are correctly calculated (it was horribly wrong before, thanks for catching it!) I'll close this first in favor of smaller and more targeted PRs.
Regarding the high level plan sounds good to me, the benchmarking would be very interesting to see how far we can get with assimp and if there would be significant tradeoffs to it becoming the default, we can continue the high level logic discussion in the issue ticket

@onurtore
Copy link
Contributor Author

@luca-della-vedova,
Perfect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants