-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Open usdview without specifying a file #3321
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
Open usdview without specifying a file #3321
Conversation
|
Requested a signed CLA from my employer. Will send the CLA to Pixar and update this PR as soon as I receive the CLA. |
|
Filed as internal issue #USD-10199 ❗ Please make sure that a signed CLA has been submitted! |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
spiffmon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one suggestion, though others may have feedback also - thanks!
| def _getRecommendedFilenamePrefix(self): | ||
| return (self._parserData.usdFile.rsplit('.', 1)[0] | ||
| if self._parserData.usdFile | ||
| else 'new_file') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something unique, like when we are creating temp files? It's probably unlikely, but if a bunch of "file-less" usdview's are launched and you do live-editing and want to save out the overrides from more than one, if you go with the recommendation you'll stomp your results. OTOH, you will see that the suggestion is the same for both, and have the ability to change it. Just a thought to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... if you do go that route, given that there's multiple uses of this recommended name, and only one "empty/new stage" per usdview, Seems like we would want just one unique name per-process, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! The unique names usually look a bit ugly to people, so I wasn't sure about it :). I can replace this function with the following if people think this would be better.
def _getRecommendedFilenamePrefix(self):
return (self._parserData.usdFile.rsplit('.', 1)[0]
if self._parserData.usdFile
else f'new_file_{os.getpid()}_{datetime.now().strftime("%Y%m%d_%H%M%S_%f")[:18]}')
The above function would make a unique name suggestion based on process ID and current datetime (millisecond precision), so almost unique everytime. But could be a bit jarring to look at. Open to suggestions :)
Here's an example of a potential suggestion from the above function:
new_file_12345_20240927_154510_12
We can potentially remove the datetime portion if we just need a unique name per process ID.
Thanks for the feedback, lmk if anything else stands out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead with the current naming, since file saving does ask for a confirmation if you try to override an existing file
|
Hi @MishDivyAmzn, thanks for the PR! Any updates on the CLA? |
|
The CLA is still in process by the legal. It should be ready very soon. I'll update this as soon as I hear back. |
|
Update: Pixar should have received the CLA now :) |
anwang2009
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MishDivyAmzn thanks for your patience! We've added a few comments, when you get the chance to take a look.
| def _getRecommendedFilenamePrefix(self): | ||
| return (self._parserData.usdFile.rsplit('.', 1)[0] | ||
| if self._parserData.usdFile | ||
| else 'new_file') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead with the current naming, since file saving does ask for a confirmation if you try to override an existing file
| def _saveOverridesAs(self): | ||
| recommendedFilename = self._parserData.usdFile.rsplit('.', 1)[0] | ||
| recommendedFilename += '_overrides.usd' | ||
| recommendedFilename = self._getRecommendedFilenamePrefix() + '_overrides.usd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below at os.path.samefile(saveName, self._parserData.usdFile):
With an empty stage, if we want to save overrides to an existing file, self._parserData.usdFile will be None and an error occurs.
| stage = self._getStageForFile(usdFilePath, sessionFilePath, | ||
| populationMaskPaths, muteLayersRe) | ||
|
|
||
| stage.SetEditTarget(stage.GetSessionLayer()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like saving overrides saves the root layer only. If someone opens an empty stage, makes some edits via the Interpreter, then does "Save Overrides As", nothing will get written out. We should only create a session layer and set it as the edit target when we have a real stage, and avoid creating the session layer for the empty stage.
|
Thank you for the feedback, I'll update this PR asap. :) |
|
@MishDivy - any chance you could find some time to update this PR? We'd also like to be able to open UsdView with no args... |
|
Apologies for the delay, this slipped out of my radar. Let me rebase this PR to latest, and fix any issues pointed out. |
Signed-off-by: Divyansh Mishra <[email protected]>
4b6c116 to
72519e9
Compare
Update SummaryI have updated this PR to address previous review comments:
TestingI verified the changes by:
|
| def _openStage(self, usdFilePath, sessionFilePath, | ||
| populationMaskPaths, muteLayersRe): | ||
|
|
||
| if self._mallocTags != 'none': | ||
| Tf.MallocTag.Initialize() | ||
|
|
||
| if not usdFilePath: | ||
| stage = self._getEmptyStage() | ||
|
|
||
| else: | ||
| stage = self._getStageForFile(usdFilePath, sessionFilePath, | ||
| populationMaskPaths, muteLayersRe) | ||
| stage.SetEditTarget(stage.GetSessionLayer()) | ||
|
|
||
| if self._mallocTags == 'stage': | ||
| DumpMallocTags(stage, "stage-loading") | ||
|
|
||
| return stage | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this PR is missing a lot of logic from the _openStage method, perhaps a bad merge? -
OpenUSD/pxr/usdImaging/usdviewq/appController.py
Line 1206 in 43b8c71
| def _openStage(self, usdFilePath, sessionFilePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment!
I renamed the old _openStage function to _getStageForFile and now the _openStage calls _getStageForFile or _getEmptyStage based on whether usdFile was passed as an argument. This way the callers of _openStage are unaffected.
Please let me know if there are any other concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh sorry, missed that. thanks!
Running
usdviewwithoutusdFilepositional argument will now open an empty stage. IfusdFileis passed, then there will be no behavior change.Description of Change(s)
usdFilepositional argement OptionalGetResolverContextmethod to return a default context whenusdFileis not passed.appController_openStagemethod to create an empty stage whenusdFilePathis a falsy value._saveFlattendedAs,_saveOverridesAsand_saveViewerImagemethods to use "new_scene" prefix for recommended file name whenusdFileis a falsy value.Note: Prior to this change, running
usdviewwithoutusdFileargument would printerror: the following arguments are required: usdFile.Fixes Issue(s)
#3093