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

Unexpected behavior of expression-mode collection with explicitOnly expansion rule #3423

Open
marktucker opened this issue Nov 13, 2024 · 8 comments

Comments

@marktucker
Copy link
Contributor

Description of Issue

I noticed some behavior that seemed odd to me when using expression-mode collections (specifically, I was testing with the kitchen scene and a path expression of //Cheerio*). With the expansion rule set to explicitOnly, I expected to match all prims named Cheerio* anywhere in the scene. Instead I got back an empty result. Then I found this in the docs:

explicitOnly - in relationship-mode, only paths in the includes relationship targets and not those in the excludes relationship targets belong to the collection. Does not apply to expression-mode. If set in expression-mode, the functions UsdComputeIncludedObjectsFromCollection() and UsdComputeIncludedPathsFromCollection() return no results.

Why has this limitation been imposed? I can see this rule made explicit in the code in _ComputeIncludedImpl of usd/collectionMembershpiQuery.cpp, the last major block inthat function does:

    // Walk everything according to \p pred, and do incremental search.
    TfToken expansionRule = query.GetTopExpansionRule();
    if (query.HasExpression() &&
        (expansionRule == UsdTokens->expandPrims ||
         expansionRule == UsdTokens->expandPrimsAndProperties)) {

Basically I'm trying to understand why this limitation would exist, and hoping that the rule can be changed so explicitOnly does what I would consider "the expected thing".

I'm happy to attempt a PR if this is an acceptable change.

Package Versions

24.08

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10441

@marktucker
Copy link
Contributor Author

I re-read the docs recently and discovered that my confusion stems in part from the fact that expandPrims in expression mode also doesn't behave as I'd expect. If I am reading it right, expandPrims behaves as I would expect explicitOnly to behave, which arguably could explain why explicitOnly does nothing. And I understand that expression mode makes it relatively easy to write an expression that is equivalent to relationship mode expandPrims behavior. But I still find this quite unexpected. To be specific, in relationship mode:

(includes = /foo/bar, explicitOnly) == [ /foo/bar ]
(includes = /foo/bar, expandPrims) == [ /foo/bar, /foo/bar/baz, /foo/bar/other, .... ]
(expr = /foo/bar, explicitOnly) == [ ]
(expr = /foo/bar, expandPrims) == [ /foo/bar ]

I hope this clarifies why I find this behavior confusing, even in light of this recent upgrade to my understanding.

@gitamohr
Copy link
Contributor

gitamohr commented Nov 26, 2024

Yeah, explicitOnly is actually doc'd that it does not to apply to expression-mode collections, and that you get no results in this case.

Does not apply to expression-mode. If set in expression-mode, the functions UsdComputeIncludedObjectsFromCollection() and UsdComputeIncludedPathsFromCollection() return no results.

I think the reasoning was that with a pattern, it was unclear what "explicit" would mean?

@gitamohr gitamohr reopened this Nov 26, 2024
@gitamohr
Copy link
Contributor

Yeesh, I did not mean to close this, sorry -- had some kind of mouse freak-out.

Anyway I agree that this is confusing. Though I'm not sure what the right solution is. Calling out expressions that have just a single pattern that's an explicit path seems weird. Same with expressions that are ors of explicit path patterns? Honestly a pattern like that already behaves like explicitOnly.

@gitamohr
Copy link
Contributor

Just to be a bit more concrete, in explicitOnly mode, an expression like /foo/bar should obviously just match /foo/bar. And certainly an expression like /foo/bar /foo/baz should match either of those two. But then say you modify the expression to be /foo/bar /foo/b*z. Should that match only /foo/bar? Should it match nothing? We could do something where we only consider expressions that are disjunctions of patterns that do not contain wildcards or stretch (//) elements. But I'm not sure that's a whole lot less confusing or more useful, and it's definitely more complicated.

@gitamohr
Copy link
Contributor

Another option would be to treat explicitOnly in expression mode as the same as either expandPrims or expandPrimsAndProperties.

@marktucker
Copy link
Contributor Author

Hey @gitamohr, I'm definitely interpreting some of these words differently than you are. To me "explicitlyOnly" mean "only return prims that explicitly match this pattern". So there is no doubt in my mind that in "explicitOnly" mode "/foo/b*r" would match "/foo/bar". The "explicitness" has nothing to do with the patterns that make up the expression. This differs from "expandPrims" which (to me) means "match any prim that either matches the pattern or is a descendant of a prim that matches the pattern" (i.e. the set of all prims returned if we were in "explicitOnly" mode plus all the descendants of those prims). This is how things work when you put a path in the "includes" relationship in relationship-mode. The "expand" refers to "expand down the hierarchy". So I expected expression mode to interpret these expansion modes the same way - "expand" means "expand down the hierarchy".

That said, I really don't have a use case for changing the interpretation of these modes (other than my claim that "it makes sense to me"). So I would be perfectly happy if "explicitOnly" === "expandPrims" in expression-mode. Leaving it the way it is now just feels like a bit of a trap which doesn't serve any useful purpose that I can see.

@gitamohr
Copy link
Contributor

Ah I see, yeah we don't treat /foo/bar as matching /foo/bar/child in expandPrims mode, because that's what /foo/bar// is for -- it lets you specify that you want descendants to match or not in a more fine-grained per-pattern way rather than globally.

Spiff is also on board with treating explicitOnly the same as expandPrims in expression mode, so I think we'll go that direction. Thanks for talking this through!

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

No branches or pull requests

3 participants