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

Draft Feature: Time measurement and Fallback system for imported meshes #360

Closed
wants to merge 4 commits into from
Closed

Draft Feature: Time measurement and Fallback system for imported meshes #360

wants to merge 4 commits into from

Conversation

onurtore
Copy link
Contributor

Uses chrono steady_clock to measure the clock time during mesh import. This will be used for the report for assimp.

@onurtore onurtore changed the title Otore19/assimp sandbox clock Time measurement for imported meshes May 22, 2022
@onurtore
Copy link
Contributor Author

onurtore commented May 22, 2022

@luca-della-vedova ,

I will use this place to share the measurement results of default and assimp loaders for different meshes.

@onurtore onurtore changed the title Time measurement for imported meshes Draft Feature: Time measurement for imported meshes May 22, 2022
@onurtore onurtore changed the title Draft Feature: Time measurement for imported meshes Draft Feature: Time measurement and Fallback system for imported meshes May 23, 2022
@onurtore
Copy link
Contributor Author

onurtore commented May 23, 2022

Hi @ahcorde, this question is about 'What Gazebo should and shouldn't do' rather than simple implementation problem. That's why I referred you.

@luca-della-vedova,
If OSRF decides to use Assimp library rather than the default ones, there is a high change that some of the meshes that Gazebo succesfully loaded before will not be loaded with the Assimp due to reasons that we don't know yet. This problem will affect end users because the worlds they loaded before cant be used anymore.

We have couple of design choices to solve this issue:

  1. If failure happens during loading with assimp, does not care at all and just print error message.
  2. If failure happens during loading with assimp, try to use one of the old loaders.
  3. If failure happens during loading with assimp, try to use one of the old loaders, further send the mesh file to gazebo servers for inspection.
  4. If failure happens during loading with assimp, try to use one of the old loaders, if there isn't any compatible ones exist use an offline/online converter for that mesh file, and further send the mesh file to gazebo servers for inspection (My favorite).

There are couple of questions here such as whether can we take user's mesh files or how hard gazebo should work to give smooth experience.

@luca-della-vedova
Copy link
Member

I agree that this is a tough problem that I honestly don't know how to solve.
The ideal scenario that I personally would like to push for is making sure that assimp works and we don't need to use any of the default loaders anymore. There are a few cases now (and there will be more in the future) of assimp not quite working perfectly with some of our current assets, my instinct for these cases would be:

  • If possible, find the issue at the assimp level and fix it there. For example while parsing actors I found out that there was a bug in the way assimp was parsing collada's SID fields that made it impossible to map bones to their node, submitted a PR to the upstream assimp that is simple enough that I believe should be merged soon.
  • If not possible or too slow, fork / vendor assimp and implement our needed changes there, while working to upstream them in the main library so we can remove the vendored / forked version.
  • If again not possible make sure it's not an asset issue, there might be a chance the assets can be re-exported in a way that fixes the issue (i.e. different field names in collada, or a different format).
  • If finally this is also not possible... Just use assimp for the new formats and the custom loaders for the older formats.

My hope is that we can cover enough in the first two cases that won't need a legacy loader approach (and maintain yet another loader!), also if our changes to assimp don't break API / ABI it should in theory be possible to release updated binaries in a released distro (i.e. Ubuntu 22.04) so we wouldn't need to vendor / fork the package.
Again this is really my 2c but thought I'd share.

This pull request was closed.
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