Skip to content

Commit

Permalink
Deprecate negative offset scale during composition
Browse files Browse the repository at this point in the history
Cummulative negative layer offset scale on a composed layer doesn't make
sense, as it reverses the direction of time, and can lead to incorrect
and non-intuitive results specially with various spline evaluation
behaviors and with time samples with soon to be implemented preValue
support.

Introduced PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE environment variable,
which currently defaults to true, allowing the use of negative layer offset,
however will issue a warning. A false value, will result in a coding error.

(Internal change: 2357711)
  • Loading branch information
tallytalwar authored and pixar-oss committed Feb 20, 2025
1 parent e104b33 commit fa8e2b2
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 3 deletions.
15 changes: 15 additions & 0 deletions pxr/usd/pcp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pxr_test_scripts(
testenv/testPcpResolvedPathChange.py
testenv/testPcpStreamingLayerReload.py
testenv/testPcpCompositionResults.py
testenv/testPcpNegativeOffsetScale.py
)

pxr_build_test_shared_lib(TestPcpDynamicFileFormatPlugin
Expand Down Expand Up @@ -2162,3 +2163,17 @@ pxr_register_test(testPcpLayerRelocatesEditBuilder
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testPcpLayerRelocatesEditBuilder"
EXPECTED_RETURN_CODE 0
)

pxr_register_test(testPcpNegativeOffsetScaleAllowed
PYTHON
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testPcpNegativeOffsetScale"
EXPECTED_RETURN_CODE 0
)

pxr_register_test(testPcpNegativeOffsetScale
PYTHON
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testPcpNegativeOffsetScale"
EXPECTED_RETURN_CODE 0
ENV
PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE=0
)
33 changes: 31 additions & 2 deletions pxr/usd/pcp/layerStack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ TF_DEFINE_ENV_SETTING(
"non-USD caches/layer stacks; the legacy behavior cannot be enabled in USD "
"mode");

TF_DEFINE_ENV_SETTING(
PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE, true,
"Enables the use of negative layer offset scale. This behavior is "
"deprecated and a warning will be issued if this setting is enabled, "
"otherwise a coding error will be issued. Negative layer offset scale on a "
"composed property doesn't make sense, as it reverses the direction of "
"time, and can lead to incorrect and non intuitive results.");


struct Pcp_SublayerInfo {
Pcp_SublayerInfo() = default;
Pcp_SublayerInfo(const SdfLayerRefPtr& layer_, const SdfLayerOffset& offset_,
Expand Down Expand Up @@ -1649,6 +1658,14 @@ PcpLayerStack::_Compute(const std::string &fileFormatTarget,
}
}

bool
PcpNegativeLayerOffsetScaleAllowed()
{
static bool allowed =
TfGetEnvSetting(PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE);
return allowed;
}

SdfLayerTreeHandle
PcpLayerStack::_BuildLayerStack(
const SdfLayerHandle & layer,
Expand Down Expand Up @@ -1786,8 +1803,20 @@ PcpLayerStack::_BuildLayerStack(

// Check sublayer offset.
SdfLayerOffset sublayerOffset = sublayerOffsets[i];
if (!sublayerOffset.IsValid()
|| !sublayerOffset.GetInverse().IsValid()) {

const bool isNegativeScale = sublayerOffset.GetScale() < 0.0;
const bool negativeScaleAllowed = PcpNegativeLayerOffsetScaleAllowed();

if (isNegativeScale && negativeScaleAllowed) {
// Report warning.
TF_WARN("Layer @%s@ has a negative offset scale. Negative scale "
"offsets are deprecated.",
layer->GetIdentifier().c_str());
}

if ((isNegativeScale && !negativeScaleAllowed) ||
!sublayerOffset.IsValid() ||
!sublayerOffset.GetInverse().IsValid()) {
// Report error, but continue with an identity layer offset.
PcpErrorInvalidSublayerOffsetPtr err =
PcpErrorInvalidSublayerOffset::New();
Expand Down
10 changes: 10 additions & 0 deletions pxr/usd/pcp/layerStack.h
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,16 @@ std::ostream& operator<<(std::ostream&, const PcpLayerStackPtr&);
PCP_API
std::ostream& operator<<(std::ostream&, const PcpLayerStackRefPtr&);

/// Returns true if negative layer offsets and scales are allowed.
///
/// Negative layer offset scales are deprecated and a warning will be issued
/// when commulative scale during composition is negative with
/// PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE is set to true (default right now).
/// If PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE is set to false, a coding error
/// will be issued when a negative scale is encountered.
PCP_API
bool PcpNegativeLayerOffsetScaleAllowed();

/// Checks if the source and target paths constitute a valid relocates. This
/// validation is not context specific, i.e. if this returns false, the
/// combination of source and target paths is always invalid for any attempted
Expand Down
14 changes: 13 additions & 1 deletion pxr/usd/pcp/primIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2123,8 +2123,20 @@ _EvalRefOrPayloadArcs(PcpNodeRef node,
fail = true;
}

const bool isNegativeScale = layerOffset.GetScale() < 0.0;
const bool negativeScaleAllowed = PcpNegativeLayerOffsetScaleAllowed();

if (isNegativeScale && negativeScaleAllowed) {
TF_WARN("Found negative scale in layer offset for %s to @%s@<%s>. "
"Negative offset scale is deprecated.",
ARC_TYPE == PcpArcTypePayload ? "payload" : "reference",
info.authoredAssetPath.c_str(),
refOrPayload.GetPrimPath().GetText());
}

// Validate layer offset in original reference or payload.
if (!layerOffset.IsValid() ||
if ((isNegativeScale && !negativeScaleAllowed) ||
!layerOffset.IsValid() ||
!layerOffset.GetInverse().IsValid()) {
PcpErrorInvalidReferenceOffsetPtr err =
PcpErrorInvalidReferenceOffset::New();
Expand Down
66 changes: 66 additions & 0 deletions pxr/usd/pcp/testenv/testPcpNegativeOffsetScale.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#!/pxrpythonsubst
#
# Copyright 2025 Pixar
#
# Licensed under the terms set forth in the LICENSE.txt file available at

import unittest

from pxr import Pcp, Sdf, Tf

def _ComposeLayersWithNegativeOffsetScale():
refLayer = Sdf.Layer.CreateAnonymous("ref")
refLayer.ImportFromString('''
#sdf 1.4.32
def "prim" {
double attr.timeSamples = {
0: 1.0,
1: 2.0,
}
}
'''.strip())

subLayer = Sdf.Layer.CreateAnonymous("sub")
subLayer.ImportFromString(f'''
#sdf 1.4.32
(
subLayers = [
@{refLayer.identifier}@ (scale = -1)
]
)
'''.strip())

rootLayer = Sdf.Layer.CreateAnonymous()
rootLayer.ImportFromString(f'''
#sdf 1.4.32
def "prim" (
references = @{subLayer.identifier}@</prim> (scale = -1)
)
{{
}}
'''.strip())

pcpCache = Pcp.Cache(Pcp.LayerStackIdentifier(rootLayer))
_, errs = pcpCache.ComputePrimIndex("/prim")
return errs

class TestPcpNegativeLayerOffsetScale(unittest.TestCase):

# Following will not result in a composition error
@unittest.skipIf(not Tf.GetEnvSetting("PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE"),
"Allow negative layer offset scale, no composition error")
def test_NegativeLayerOffsetScaleAllowed(self):
errs = _ComposeLayersWithNegativeOffsetScale()
self.assertEqual(len(errs), 0)

# Following will result in a composition error
@unittest.skipIf(Tf.GetEnvSetting("PCP_ALLOW_NEGATIVE_LAYER_OFFSET_SCALE"),
"Do not allow negative layer offset scale, composition error")
def test_NegativeLayerOffsetScaleNotAllowed(self):
errs = _ComposeLayersWithNegativeOffsetScale()
self.assertEqual(len(errs), 2)

if __name__ == "__main__":
unittest.main()

0 comments on commit fa8e2b2

Please sign in to comment.