Skip to content

[HM3D]--Semantic scene support/bugfixes.#1592

Merged
jturner65 merged 22 commits intofacebookresearch:mainfrom
jturner65:SemanticSceneFixes
Dec 14, 2021
Merged

[HM3D]--Semantic scene support/bugfixes.#1592
jturner65 merged 22 commits intofacebookresearch:mainfrom
jturner65:SemanticSceneFixes

Conversation

@jturner65
Copy link
Copy Markdown
Contributor

Motivation and Context

This PR adds various functions and refactors required to implement support for HM3D semantic scenes in the correct format. Included are :

  • Better vertex color handling in semantic mesh load
  • More rigorous string verification for sentinel string denoting Semantic Scene Descriptor file type
  • Proper application of frame re-orientation to Semantic Mesh data. This is required to enable any semantic meshes to be re-oriented in engine.
  • Force all semantic meshes to be treated as Instance Meshes (temporary so that vertex colors and vertex ids are supported).

Also, some refactors to improve efficiency and debug message content in Resource Manager are included.

How Has This Been Tested

Locally c++ and python tests

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 8, 2021
@jturner65 jturner65 requested a review from aclegg3 December 8, 2021 22:00
importerManager_.setPreferredPlugins("ObjImporter", {"AssimpImporter"});
Cr::PluginManager::PluginMetadata* const assimpmetadata =
importerManager_.metadata("AssimpImporter");
assimpmetadata->configuration().setValue("ImportColladaIgnoreUpDirection",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, I wish there was a way to use this if it wasn't specified in the JSON...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you explain what you mean? Thanks :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am worried the fact that other importers like Blender may support Collada's up direction may cause confusion for end-users when the rotations do not match. Seems like it's harmful to ignore an up direction defined in the dae. What about other formats that have their own up direction, it feels like we should try to be consistent with the handling of the up direction across formats. (For instance, the rotation in the dataset should be relative to the up direction in the mesh file).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This flag is consistent with the current engine treatment (no change from this PR), so let's not block on this.

Context: It was necessary to get consistent behavior across a variety of URDF files. I agree we should re-visit this and ensure our handling is consistent with other importers in a later PR.

@jturner65 jturner65 force-pushed the SemanticSceneFixes branch 4 times, most recently from 53fa342 to 9af3d7a Compare December 13, 2021 18:25
Copy link
Copy Markdown
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.

LGTM

@jturner65 jturner65 merged commit b4a060f into facebookresearch:main Dec 14, 2021
@jturner65 jturner65 deleted the SemanticSceneFixes branch December 14, 2021 21:27
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