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

Fix GetDisplayName() for Anonymous Layers. #440

Merged

Conversation

superfunc
Copy link
Contributor

@superfunc superfunc commented Mar 24, 2018

Description of Change(s)

It was broken when the provided identifier had a colon in it,
which is legal in a posix file path.

e.g.

from pxr import Usd
s = Usd.Stage.CreateInMemory('foo:foo.usda')
s.GetRootLayer().GetDisplayName()

Should return foo:foo.usda, but currently returns an empty string

Fixes Issue(s)

  • None

@jtran56
Copy link

jtran56 commented Mar 26, 2018

Filed as internal issue #158778.

Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

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

Hey @superfunc, I think this change seems fine, but iI think the test case should be in Sdf itself. Could you add a test case in testSdfLayer.py as well? Thanks!

@superfunc
Copy link
Contributor Author

Yeah I'll get that up tonight ✌️

@sunyab sunyab added blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed and removed in review labels Apr 21, 2018
@superfunc
Copy link
Contributor Author

Ok, I just moved the test over to sdf, should be good to merge now.

Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

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

Sorry, there was one last review note I missed last time. Thanks!

return std::string();
// We want to find the second occurence of ':', traversing from the left,
// in our identifier which is of the form anon:0x4rfs23:displayName
auto fst = std::find(identifier.begin(), identifier.end(), ':');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we bail here if fst == identifier.end() for safety? The old code handled the case where there were no ":" characters in the identifier (even if this should never happen), so I think it'd be good to follow suit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, updated. Thanks :)

It was broken when the provided identifier had a colon in it,
which is legal in a posix file path.
This behavior fundamentally lives in sdf, so it should be tested
there. UsdStage's constructors just forward into this code.
@sunyab sunyab added pending push and removed blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed labels May 17, 2018
@pixar-oss pixar-oss merged commit e0d6867 into PixarAnimationStudios:dev May 21, 2018
pixar-oss added a commit that referenced this pull request May 21, 2018
Fix GetDisplayName() for Anonymous Layers.
@superfunc superfunc deleted the sdf_layer_display_name_fix branch May 21, 2018 18:31
AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
…Controller. (PixarAnimationStudios#440)

### Description of Change(s)
Move setting the camera and framing information in RenderIndex from UsdImagingGLEngine to taskController.

This is because UsdImagingGLEngine may not be used by the client. But taskController is more widely used.

Also add SetCameraFramingState function to RenderIndex. Sometimes there is no camera set when the renderindex syncs, so the user must set worldToViewMatrix, projection matrix and viewport to the renderindex directly.
### Fixes Issue(s)
-

<!--
Please follow the Contributing and Building guidelines to run tests against your
change. Place an X in the box if tests are run and are all tests passing.
-->
- [ ] I have verified that all unit tests pass with the proposed changes
<!-- 
Place an X in the box if you have submitted a signed Contributor License Agreement.
A signed CLA must be received before pull requests can be merged.
For instructions, see: http://openusd.org/release/contributing_to_usd.html
-->
- [ ] I have submitted a signed Contributor License Agreement
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