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

Don't give up when encountering lines/points/... in collision mesh data #1888

Merged
merged 4 commits into from
Nov 7, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Oct 5, 2022

Motivation and Context

Instead just skipping them. See this Slack thread for background info.

As this makes the isMeshPrimitiveValid() check obsolete, I removed the whole implementation of it (which, after all, was just a very verbose way to print the mesh primitive name originating back in the days when Habitat used glog and thus couldn't just call Magnum's builtin output operator for MeshPrimitive). Furthermore, a call to this function was also the only reason why addStage() too the collision mesh group parameter, so I removed it from there as well. Which is a documentation TODO:

  • The addStage() docs probably need to be updated as they mention collision data that are no longer passed there. I don't know what should be said there, tho.

The check is now moved to inside constructBulletSceneFromMeshes(), where it just skips unsupported primitives with a warning. If the code could access the originating Magnum Trade::MeshData instead of
CollisionMeshData, it could be easily extended to support also triangle fans and triangle strips (since there's a MeshTools utility for converting them to triangle lists), but right now it skips those as well.

Also contains a drive-by fix for viewer --help output and use of a deprecated Magnum API.

How Has This Been Tested

⚠️ I have no idea what am I doing! The viewer doesn't assert for me anymore, but I don't know anything about the physics code, how it should / should not behave, and how to verify if the collision mesh is indeed generated correctly. So please test and give feedback. Making the PR as a draft due to that reason (and due to the doc TODO above).

@mosra mosra requested a review from aclegg3 October 5, 2022 17:53
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 5, 2022
Instead just skipping them.

As this makes the isMeshPrimitiveValid() check obsolete, I removed the
whole implementation of it (which, after all, was just a very verbose
way to print the mesh primitive name, and could be done in a single
expression instead). Furthermore, a call to this function was also the
only reason why addStage() too the collision mesh group parameter, so I
removed it from there as well.

The check is now moved to inside constructBulletSceneFromMeshes(), where
it just skips unsupported primitives with a warning. If the code could
access the originating Magnum::Trade::MeshData instead of
CollisionMeshData, it could be easily extended to support also triangle
fans and triangle strips (since there's a MeshTools utility for
converting them to triangle list), but right now it skips those as well.
I was looking for it in the --help output and it wasn't there since it
wasn't documented. Bad usability.
@mosra mosra force-pushed the physics-lines-are-not-fatal branch from 48c85bb to b02886b Compare November 5, 2022 12:42
@mosra mosra marked this pull request as ready for review November 5, 2022 14:19
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

This fix looks good to me. Thanks! 👍

@mosra mosra merged commit c888518 into main Nov 7, 2022
@mosra mosra deleted the physics-lines-are-not-fatal branch November 7, 2022 19:49
@mukulkhanna
Copy link
Contributor

Hey @mosra @aclegg3 , I just noticed that even though these changes are allowing the scenes to be loaded, the navmesh construction is still faulty.

I have identified a list of (at least) 28 scenes with this issue. Here are scene IDs of two of those: 102816627, 102344403. The habitat-viewer's navmesh visualization for these scenes shows how the navmesh is being created for some non-existent surfaces on the ceiling (and almost nothing for the actual floor):

Screenshot 2022-11-13 at 2 21 21 PM image

This should be easy to reproduce using the habitat-viewer with the main branch:

./build/viewer --enable-physics --stage-requires-lighting --recompute-navmesh --dataset /path/to/hab-fp.scene_dataset_config.json -- 102816627

@mosra
Copy link
Collaborator Author

mosra commented Nov 19, 2022

As I said above,

⚠️ I have no idea what am I doing

unfortunately I know too little about this area of the codebase to be able to understand what's going on 😅 I only made a best-intention attempt to not discard everything when some of the meshes are not triangles, and that part, to my understanding, should be working. If it means some new meshes, which were completely skipped before, are now showing up in the navmesh generation, there's possibly some additional problem. But also I may have overlooked something important that the original code was doing and that the new code isn't anymore, which is why I'm deferring to people who actually know the navmesh generation code, such as Alex.

Scenes that loaded correctly before are loading correctly now as well? Or neither that? Do you see any

Unsupported collision mesh primitive , skipping

messages in the output for the scenes that load wrong?

@mukulkhanna
Copy link
Contributor

Thanks @mosra.

Scenes that loaded correctly before are loading correctly now as well?

Yes!

Do you see any "Unsupported collision mesh primitive , skipping" messages in the output for the scenes that load wrong?

Yes, I do.

@aclegg3
Copy link
Contributor

aclegg3 commented Nov 28, 2022

Hey @mosra @aclegg3 , I just noticed that even though these changes are allowing the scenes to be loaded, the navmesh construction is still faulty.
...

Sorry for the long delay here.
I've investigated the issue and confirmed that this bug is related to the interpretation of the non-triangle mesh components by the navmesh computation code. My hypothesis is that recast voxelization assumes triangles, so the indices are getting clobbered resulting in artifacts.

Quick fix is to again filter by primitive during the mesh join phase of navmesh re-computation. See PR #1951. Try this out and see if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants