Skip to content

Conversation

@MishDivyAmzn
Copy link
Contributor

@MishDivyAmzn MishDivyAmzn commented Sep 27, 2024

Running usdview without usdFile positional argument will now open an empty stage. If usdFile is passed, then there will be no behavior change.

Description of Change(s)

  • Made usdFile positional argement Optional
  • Updated the GetResolverContext method to return a default context when usdFile is not passed.
  • Updated the appController _openStage method to create an empty stage when usdFilePath is a falsy value.
  • Updated the _saveFlattendedAs, _saveOverridesAs and _saveViewerImage methods to use "new_scene" prefix for recommended file name when usdFile is a falsy value.

Note: Prior to this change, running usdview without usdFile argument would print error: the following arguments are required: usdFile.

Fixes Issue(s)

#3093

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

@MishDivyAmzn
Copy link
Contributor Author

Requested a signed CLA from my employer. Will send the CLA to Pixar and update this PR as soon as I receive the CLA.

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10199

❗ Please make sure that a signed CLA has been submitted!

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@spiffmon spiffmon left a 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')
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

@MishDivyAmzn MishDivyAmzn Sep 28, 2024

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.

Copy link
Contributor

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

@anwang2009
Copy link
Contributor

Hi @MishDivyAmzn, thanks for the PR! Any updates on the CLA?

@MishDivyAmzn
Copy link
Contributor Author

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.

@MishDivyAmzn
Copy link
Contributor Author

Update: Pixar should have received the CLA now :)

Copy link
Contributor

@anwang2009 anwang2009 left a 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')
Copy link
Contributor

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'
Copy link
Contributor

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())
Copy link
Contributor

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.

@MishDivy
Copy link

MishDivy commented Jan 9, 2025

Thank you for the feedback, I'll update this PR asap. :)

@pmolodo
Copy link
Contributor

pmolodo commented Oct 15, 2025

@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...

@MishDivy
Copy link

Apologies for the delay, this slipped out of my radar. Let me rebase this PR to latest, and fix any issues pointed out.

@MishDivyAmzn MishDivyAmzn force-pushed the dev_usdview_no_file_path branch from 4b6c116 to 72519e9 Compare October 16, 2025 14:46
@MishDivy
Copy link

Update Summary

I have updated this PR to address previous review comments:

  • When usdview is launched without a usdFile argument, it no longer creates a Session layer.
    All overrides are now authored to the anonymous root layer instead.
  • Fixed the issue where attempting to save overrides with the same filename would raise an error.

Testing

I verified the changes by:

  • Launching usdview with no arguments (empty stage).
  • Authoring scene description using the Python interpreter.
  • Using Save Overrides As… and Save Flattened As… to confirm that data exports correctly from usdview.
  • Reopening the saved files to ensure they load successfully and edits persist as expected.

Comment on lines +1293 to +1311
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

Copy link
Contributor

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? -

def _openStage(self, usdFilePath, sessionFilePath,

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.

Copy link
Contributor

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!

@pixar-oss pixar-oss closed this in dfbc03f Nov 5, 2025
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.

7 participants