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

-- [Part 1 of 2] Semantic Attributes Metadata : Support for Semantic Regions - Add attributes/config support #2299

Merged
merged 23 commits into from
Jan 22, 2024

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Jan 10, 2024

Motivation and Context

This PR is part 1 of a 2 part changeset that adds support for semantic regions in a dataset-agnostic way through the use of the existing metadata subsystem tooling.

This component adds manager, attributes and JSON config support for semantic scene descriptions that include region annotations for poly-loop-based extrusion region definitions, integrated into the existing metadata subsystem, managed by the existing scene dataset handling and accessed during scene creation. This PR does not introduce any breaking changes to the existing system, but does expand the possible metadata loading/handling in the scene dataset configs for the "semantic_scene_descriptor_instances" tag.

Part 2 of this PR will use these configs to instantiate Semantic Regions with the appropriate geometry and location that will support point containment checks and debug rendering.

How Has This Been Tested

Existing c++ and python tests pass.

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 Jan 10, 2024
@jturner65 jturner65 force-pushed the SemanticRegions branch 4 times, most recently from 3d6dd65 to f74d89d Compare January 11, 2024 19:38
@jturner65 jturner65 marked this pull request as ready for review January 22, 2024 15:07
@jturner65 jturner65 changed the title --[WIP] Semantic Attributes Metadata : Add Support for Semantic Regions; -- [Part 1 of 2] Semantic Attributes Metadata : Support for Semantic Regions - Add attributes/config support Jan 22, 2024
HM3D is still not working.
Loading the default/path/configs tags out of order can cause very difficult to debug issues.  They must be loaded in the given order.
TODO : need to add template bindings when format has been firmed up.
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.

Overall looks good. Once the tests pass, let's merge it.

@@ -237,6 +239,15 @@ void declareBaseAttributesManager(py::module& m,
"if it does not.")
.c_str(),
"handle"_a)
.def("get_first_matching_template_by_handle",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce this instead of getting all matches and selecting the first like we do already? Doing that puts responsibility on user code to handle the case of multiple matches and offers the opportunity for an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was doing this to provide a shortcut for folks who know they are going to use the first handle they get back, so they didn't have to deal with querying the handles and then querying for the actual attributes. Seems like the first index tends to be the goto for this kind of process, and I thought I'd abstract it away. The other mechanism is still present.

@jturner65 jturner65 merged commit a5b0db3 into main Jan 22, 2024
10 checks passed
@jturner65 jturner65 deleted the SemanticRegions branch January 22, 2024 21:12
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.

3 participants