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

[Pxr] Add UsdStage::GetObjectAtPath. #390

Merged
merged 1 commit into from
May 21, 2018

Conversation

superfunc
Copy link
Contributor

Description of Change(s)

This allows a convenient way to get items in a stage from paths,
whether they are property paths or prim paths.

Fixes Issue(s)

This was requested on the interest forum a while back, don't have the link handy.

@jtran56
Copy link

jtran56 commented Jan 29, 2018

Filed as internal issue #156422.

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.

Passing along some review notes, thanks!

/// Suggested use:
///
/// \code
/// if (UsdObject myObj = stage.GetObjectAtPath(path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just use UsdObject::As in this example, since As is documented to return an invalid object if it's called on an invalid object, like:

if (UsdObject obj = stage->GetObjectAtPath(path)) {
    if (UsdPrim prim = obj.As<UsdPrim>()) {
        // Do things with prim
    }
    else if (UsdProperty prop = obj.As<UsdProperty>()) {
        // Do things with property. We can also cast to
        // UsdRelationship or UsdAttribute using this same pattern.
    }
}
else {
    // No object at specified path
}

/// proxy prim if a prim exists at the corresponding path in that instance's
/// master.
///
/// Suggested use:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of 'Suggested use:', can we say 'Example:' instead?

}

bool isPrimPath = path.IsPrimPath();
bool isPropPath = path.IsPropertyPath();
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 make these bools const? Also, can we avoid calling IsPropertyPath() if isPrimPath is already true:

const bool isPrimPath = path.IsPrimPath();
const bool isPropPath = !isPrimPath && path.IsPropertyPath();

}

// A valid prim must be found to return either a prim or prop
auto prim = GetPrimAtPath(path.GetPrimPath());
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 avoid calling GetPrimPath() on paths we already know are prim paths?

auto prim = GetPrimAtPath(path.GetPrimPath());
if (prim) {
if (isPrimPath) {
return prim.As<UsdObject>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to explicitly cast to UsdObject using As() here and below, the implicit derived-to-base conversion should be sufficient.

@@ -700,6 +700,33 @@ class UsdStage : public TfRefBase, public TfWeakBase {
USD_API
UsdPrim GetPrimAtPath(const SdfPath &path) const;

/// Return the UsdObject at \p path, or an invalid UsdObject if none exists.
///
/// If \p path indicates a prim beneath an instance, returns an instance
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 add to this doc to mention properties and instance proxies as well? Something like,

If \p path indicates a property beneath a child of an instance, returns a property
whose parent prim is an instance proxy prim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the notes, I'll get an updated branch pushed up this weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay @sunyab, updated

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 for the delay, just have a few small follow-up notes. Thanks!


// A valid prim must be found to return either a prim or prop
if (isPrimPath) {
if (auto prim = GetPrimAtPath(path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just return GetPrimAtPath(path) here and avoid unnecessarily checking the validity of the prim here.

}
} else if (isPropPath) {
if (auto prim = GetPrimAtPath(path.GetPrimPath())) {
if (auto prop = prim.GetProperty(path.GetNameToken())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar here; we need to check the validity of the prim, but we should be able to just return prim.GetProperty(...) and not need to check the validity of the property object before returning it.

@sunyab sunyab added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Apr 27, 2018
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.

testUsdPrims is currently failing on the asserts I commented on below. I tested the fixes I noted in the comments and verified they work, so if you can make these changes I should be able to (finally) merge this in. Thanks!

# check for prims/props that dont exist
p = s.GetObjectAtPath(u'/nonexistent')
assert not p
assert type(p) is Usd.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

These asserts are failing now. I think this assert needs to be:

assert type(p) is Usd.Prim


a = s.GetObjectAtPath(u'/foo.nonexistentattr')
assert not a
assert type(a) is Usd.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

assert type(p) is Usd.Property


r = s.GetObjectAtPath(u'/foo.nonexistentrelationship')
assert not r
assert type(r) is Usd.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

assert type(r) is Usd.Property

@superfunc
Copy link
Contributor Author

Sorry about that! I forgot CI isn't running the tests, I'll get this pushed in a few

This allows a convenient way to get items in a stage from paths,
whether they are property paths or prim paths.
@superfunc
Copy link
Contributor Author

Ok @sunyab should be good to go. I also smashed it down into one commit.

@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 21, 2018
@pixar-oss pixar-oss merged commit 069c4b1 into PixarAnimationStudios:dev May 21, 2018
pixar-oss added a commit that referenced this pull request May 21, 2018
[Pxr] Add UsdStage::GetObjectAtPath.
@superfunc superfunc deleted the stagegetobjectatpath branch May 21, 2018 18:31
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