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

Replace boost::multi_index_container with custom container in Sdf_LayerRegistry #2353

Merged

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Mar 22, 2023

Description of Change(s)

To reduce the number of dependencies on boost, this PR introduces a custom container that synchronizes a set of unordered maps. Internally, it leverages layer Sdf_AssetInfo as the generator of keys for Insert, Update, and Erase operations.

Fixes Issue(s)

N/A

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-8140

public:
_Layers() = default;
using LayersByRealPath =
std::unordered_map<std::string, SdfLayerHandle, TfHash>;
Copy link

Choose a reason for hiding this comment

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

I don't believe that LayersByIdentifier and LayersByRepositoryPath are going to be equivalent to the original behavior if implemented as std::unordered_map as the multi index container had those two keys set as non-unique.
I'm trying to find out from internal conversations as to if/why this is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good catch! I think "one => many" can be similarly modeled with a unordered_multimap, but it'd would be good to understand the practical situations where the unordered_map implementation fails so we can get test coverage for it.

Copy link

Choose a reason for hiding this comment

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

I've learned at least one of the reasons is that you can have different identifiers resolve to the same real layer path under different ArResolverContexts. And the good news is we do have a test case for this in testSdfLayer so we should be able to catch if there's an issue pretty easily.

Copy link
Collaborator Author

@nvmkuruc nvmkuruc Jul 6, 2023

Choose a reason for hiding this comment

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

testSdfLayer passes. It seems like SdfLayer tries to avoid look ups using identifiers when they are context dependent (deferring to the real path instead) which probably explains why the code is working. It seems like the case that might fail is that if you mixed context dependent paths and non-context dependent paths and the context-dependent path boots the non-context dependent path lookup from the registry. I'll see if I can fashion that test case and move things over to use unordered_multimap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

testSdfLayer was passing because the test which exercises multiple contexts was being skipped due to

    @unittest.skipIf(preferredResolver != "ArDefaultResolver",
                     "Test uses search-path functionality specific to "
                     "ArDefaultResolver")

By default, testSdfLayer is configured to run only against a test resolver.

preferredResolver = os.environ.get(
    "TEST_SDF_LAYER_RESOLVER", "Sdf_TestResolver")

@nvmkuruc nvmkuruc force-pushed the layer_registry_multiindex branch 2 times, most recently from 7b623ca to bf1164b Compare August 4, 2023 19:16
@nvmkuruc nvmkuruc force-pushed the layer_registry_multiindex branch 4 times, most recently from e0efa8d to cab1dfa Compare November 8, 2023 17:59
@pixar-oss pixar-oss merged commit 5607339 into PixarAnimationStudios:dev Nov 17, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants