Add initial implementation of `usdSemantics` #3103

nvmkuruc · 2024-05-31T22:21:05Z

Description of Change(s)

Implementation of https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main/proposals/semantic_schema which adds a SemanticsLabelsAPI schema for labeling prims with tokens.

Note that the implementation diverges slightly with the proposal with the taxonomy being after labels instead of before in the property name. (ie. semantics:labels:<taxonomy> instead of semantics:<taxonomy>:labels).

In addition to the schema, a utility UsdSemanticsLabelsQuery provides a reference for how ancestor labeling is expected to work and inherit.

Fixes Issue(s)

I have verified that all unit tests pass with the proposed changes

I have submitted a signed Contributor License Agreement

jesschimein · 2024-06-03T16:54:54Z

Filed as internal issue #USD-9719

jesschimein · 2024-06-03T16:54:56Z

/AzurePipelines run

azure-pipelines · 2024-06-03T16:55:07Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-06-04T18:02:52Z

/AzurePipelines run

azure-pipelines · 2024-06-04T18:03:01Z

Azure Pipelines successfully started running 1 pipeline(s).

pixar-oss · 2024-06-06T22:16:32Z

/AzurePipelines run

azure-pipelines · 2024-06-06T22:16:42Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-07-22T16:38:55Z

/AzurePipelines run

azure-pipelines · 2024-07-22T16:39:05Z

Azure Pipelines successfully started running 1 pipeline(s).

pxr/usd/usdSemantics/overview.md

jesschimein · 2024-08-09T16:45:58Z

/AzurePipelines run

azure-pipelines · 2024-08-09T16:46:09Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-08-29T17:02:57Z

/AzurePipelines run

azure-pipelines · 2024-08-29T17:03:07Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-09-12T17:55:21Z

/AzurePipelines run

azure-pipelines · 2024-09-12T17:55:31Z

Azure Pipelines successfully started running 1 pipeline(s).

spiffmon · 2024-09-18T23:36:02Z

pxr/usd/usdSemantics/labelsQuery.h

+    /// Compute the union of values for `semantics:labels:<taxonomy>` in the
+    /// specified interval.


For this and other methods that return an array of labels, please specify whether there is a stable ordering of the returned values, or client must expect them to be disordered.

For this method specifically, it WBN to just reinforce the convention that "Direct" means this prim's contributions only.

spiffmon · 2024-09-18T23:40:16Z

pxr/usd/usdSemantics/labelsQuery.cpp

+
+bool
+UsdSemanticsLabelsQuery::_PopulateLabels(const UsdPrim& prim) {
+    TF_DEV_AXIOM(!prim.IsPseudoRoot());


TF_VERIFY is preferred to TF_DEV_AXIOM

spiffmon · 2024-09-18T23:45:16Z

pxr/usd/usdSemantics/labelsQuery.cpp

+UsdSemanticsLabelsQuery::_PopulateLabels(const UsdPrim& prim) {
+    TF_DEV_AXIOM(!prim.IsPseudoRoot());
+    UsdSemanticsLabelsAPI schema(prim, _taxonomy);
+    if (!schema.GetPrim().HasAPI<decltype(schema)>(schema.GetName()))


Constructing a schema object is not trivial in cost; can we do this check first, using the given prim, UsdSemanticsLabelsAPI, and _taxonomy ?

I don't think we should be doing this check at all-- we should just be validating the schema.

The HasAPI check is a faster check, though, and given that we expect, in many scenes, to have few if any applications of the schema, it may be worth doing the fast check up front?

spiffmon · 2024-09-18T23:50:01Z

pxr/usd/usdSemantics/labelsQuery.h

+/// The query caches certain reads and computations and should be discarded
+/// when the state of the stage changes.


Helpful to mention that it is safe to use in a multi-threaded context... not all caches in Usd schemas are currently, regrettably.

spiffmon · 2024-09-18T23:53:41Z

pxr/usd/usdSemantics/schema.usda

+
+class "SemanticsLabelsAPI" (
+    inherits = </APISchemaBase>
+    doc = """Applied set of labels for a prim for a taxonomy specified by the


Given implied meanings of "set", can we be specific on what we mean, here? Is it an ordered list of labels, or is authored order insignificant?

pxr/usd/usdSemantics/labelsQuery.h

jesschimein · 2024-09-19T16:54:31Z

/AzurePipelines run

azure-pipelines · 2024-09-19T16:54:41Z

Azure Pipelines successfully started running 1 pipeline(s).

pxr/usd/usdSemantics/labelsQuery.h

jesschimein · 2024-09-23T16:57:47Z

/AzurePipelines run

azure-pipelines · 2024-09-23T16:57:57Z

Azure Pipelines successfully started running 1 pipeline(s).

matthewcpp

The comments contained herein came from our internal code review.

matthewcpp · 2024-09-23T16:58:47Z

pxr/usd/usdSemantics/labelsQuery.cpp

+            return {std::cbegin(labelsAtTime), std::cend(labelsAtTime)};
+        }
+        if constexpr (std::is_same_v<TimeType, GfInterval>) {
+            TF_DEV_AXIOM(!queryTime.IsEmpty());


maybe this could use TF_VERIFY? Unless checking this condition is expensive enough to have a real impact on performance?

Sure. Shouldn't be performance critical.

matthewcpp · 2024-09-23T17:02:50Z

pxr/usd/usdSemantics/labelsQuery.cpp

+                VtTokenArray labelsAtTime;
+                if (!labelsAttr.Get(&labelsAtTime, timeInInterval)) {
+                    TF_RUNTIME_ERROR("Failed to read value at %s",
+                                     UsdDescribe(labelsAttr).c_str());


I'm curious when we would hit the two conditions here that cause runtime errors to be emitted. Are those really runtime conditions that make that happen?

(Sorry to be so focused on error policy here, rather than what I'm supposed to be weighing in on!!)

Thanks for doing so, @nvmkuruc , as I was just about to. RUNTIME_ERRORs should be reserved for system-level errors. Here and elsewhere, if we really expected there should have been something authored but there wasn't, it would be more of a TF_WARN, I think?

@matthewcpp The following layer can trigger that runtime error

def "test" (apiSchemas = ["SemanticsLabelsAPI:test"]) { int[] semantics:labels:test.timeSamples = {100: [1]} }

with the following code.

from pxr import Usd, UsdSemantics s = Usd.Stage.Open('sl.usda') query = UsdSemantics.LabelsQuery('test', 100) print(query.ComputeUniqueDirectLabels(s.GetPrimAtPath('/test')))

@spiffmon I'm happy to use whatever diagnostic makes the most sense, but it did seem like an error to me.

Right, so that's "bad scene description", similar to having something like

over "mySphere" { string radius = "five" }

which is not considered a runtime error. In Presto (and future USD) it would be a validation error, but at scene-interrogation-time, I think we are silent about it, actually.

matthewcpp · 2024-09-23T17:03:29Z

pxr/usd/usdSemantics/labelsQuery.cpp

+        // cache. Attempting to collapse this expression using |= or the
+        // STL may result in some population being skipped and incorrect
+        // results.
+        if (_PopulateLabels(stage->GetPrimAtPath(path)))


We could do this population in parallel?

matthewcpp · 2024-09-23T17:04:12Z

pxr/usd/usdSemantics/labelsQuery.cpp

+        return {};
+    }
+
+    std::shared_lock lock{_cachedLabelsMutex};


I'm curious why we need to lock here; I assume the locking is only to protect against simultaneous writes (or reads while writing is happening). But at this point, we know the cache has already been populated, so presumably it is safe to allow multiple readers to proceed in parallel?

If another thread is querying the cache, a rehash could be triggered, invalidating the iterator.

matthewcpp · 2024-09-23T17:04:54Z

pxr/usd/usdSemantics/labelsQuery.cpp

+        return {};
+    }
+    VtTokenArray result{std::cbegin(it->second), std::cend(it->second)};
+    std::sort(result.begin(), result.end());


Could we sort as part of population?

Ah, I guess we can't because we use sets so that we can combine inherited results. It seems like it would be nice to frontload the sorting, somehow, but there are tradeoffs to consider, and we'd want to actually measure performance to understand the impact, most likely.

It's possible it would be more efficient to use std::merge on sorted token arrays. But perhaps we could address later?

matthewcpp · 2024-09-23T17:06:08Z

pxr/usd/usdSemantics/labelsQuery.cpp

+
+    _UnorderedTokenSet uniqueElements;
+    {
+        std::shared_lock lock{_cachedLabelsMutex};


Again, I'm not yet understanding why we need to lock once population is complete.

See the previous note about the iterator lifetime.

nvmkuruc force-pushed the semantics branch from b91911f to 270a2b3 Compare June 4, 2024 17:33

nvmkuruc force-pushed the semantics branch from 270a2b3 to 0a1332c Compare July 19, 2024 15:05

nvmkuruc force-pushed the semantics branch from 0a1332c to 17826dd Compare August 7, 2024 21:04

nvaszabo reviewed Aug 8, 2024

View reviewed changes

pxr/usd/usdSemantics/overview.md Outdated Show resolved Hide resolved

nvmkuruc force-pushed the semantics branch 6 times, most recently from 02d4fcd to 89624aa Compare August 9, 2024 14:12

nvmkuruc mentioned this pull request Aug 19, 2024

Not able to Import Semantics from pxr #3240

Closed

nvmkuruc force-pushed the semantics branch from 89624aa to e155ccb Compare August 29, 2024 14:19

nvmkuruc force-pushed the semantics branch from e155ccb to 319cad2 Compare September 12, 2024 01:03

nvmkuruc force-pushed the semantics branch from 319cad2 to 27f71d5 Compare September 16, 2024 20:13

spiffmon reviewed Sep 18, 2024

View reviewed changes

nvmkuruc force-pushed the semantics branch from 27f71d5 to 90121ee Compare September 19, 2024 17:48

Add initial implementation of usdSemantics

bb8ac6d

nvmkuruc force-pushed the semantics branch from 90121ee to bb8ac6d Compare September 20, 2024 21:32

spiffmon reviewed Sep 21, 2024

View reviewed changes

pxr/usd/usdSemantics/labelsQuery.h Show resolved Hide resolved

matthewcpp reviewed Sep 23, 2024

View reviewed changes

pixar-oss merged commit b266dd4 into PixarAnimationStudios:dev Oct 3, 2024
5 checks passed

		/// Compute the union of values for `semantics:labels:<taxonomy>` in the
		/// specified interval.

		/// The query caches certain reads and computations and should be discarded
		/// when the state of the stage changes.

Add initial implementation of usdSemantics #3103

Add initial implementation of usdSemantics #3103

Conversation

nvmkuruc commented May 31, 2024 • edited Loading

Description of Change(s)

Fixes Issue(s)

jesschimein commented Jun 3, 2024

jesschimein commented Jun 3, 2024

azure-pipelines bot commented Jun 3, 2024

jesschimein commented Jun 4, 2024

azure-pipelines bot commented Jun 4, 2024

pixar-oss commented Jun 6, 2024

azure-pipelines bot commented Jun 6, 2024

jesschimein commented Jul 22, 2024

azure-pipelines bot commented Jul 22, 2024

jesschimein commented Aug 9, 2024

azure-pipelines bot commented Aug 9, 2024

jesschimein commented Aug 29, 2024

azure-pipelines bot commented Aug 29, 2024

jesschimein commented Sep 12, 2024

azure-pipelines bot commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Sep 19, 2024

azure-pipelines bot commented Sep 19, 2024

jesschimein commented Sep 23, 2024

azure-pipelines bot commented Sep 23, 2024

matthewcpp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add initial implementation of `usdSemantics` #3103

Add initial implementation of `usdSemantics` #3103

nvmkuruc commented May 31, 2024 •

edited

Loading