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

[sdf] Fix Sdf_Children<ChildPolicy>::Find to return correct value #3486

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nvidia-jomiller
Copy link
Collaborator

In most cases, Sdf_Children::Find would return the incorrect value of 0 instead the size of _childNames. This would incorrectly index into the first element when a match was not found

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

Checklist

[x] I have created this PR based on the dev branch

[x] I have followed the coding conventions

[x] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

[x] I have verified that all unit tests pass with the proposed changes

[x] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

In most cases, Sdf_Children<ChildPolicy>::Find would return the incorrect value of 0 instead the size of _childNames.
This would incorrectly index into the first element when a matching element was not found
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10583

(This is an automated message. See here for more information.)

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sunyab
Copy link
Contributor

sunyab commented Feb 7, 2025

Hey @nvidia-jomiller,

I'm not quite seeing the bug you mentioned in your PR description. Prior to your change, in the case where the given key isn't found, the index i would have been incremented to _childNames.size() and then returned. The test case you added worked both with and without your change. AFAICT, the only functional change is that in the failed verify case we now return _childNames.size() instead of 0.

Am I missing something?

@nvidia-jomiller
Copy link
Collaborator Author

Hey @sunyab , thanks for taking a look!

The change was mostly facilitated by another task but yeah, now that I read through it line by line I see functionally it's equivalent. What I was mostly going from is checks in SdfChildrenView and code docs in Sdf_Children. I didn't see any tests specifically testing it so I modified and added a test. I think this might just be a readability thing where I was expecting to see _childNames.size() returned. But I'm happy to close this out

@sunyab
Copy link
Contributor

sunyab commented Feb 8, 2025

Ok, thanks for confirming! Please leave this open for now, if nothing else I think we can merge in the test since more tests are always appreciated.

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.

3 participants