Fix value clips bug where incorrect time samples would

be added to the list of time samples for a clip.

For example, given a clip times metadata value like
[(0, 0), (4, 4), (5, 0), (6, 1), (7, 2), ...], we
would expect an affected attribute to have time samples
at times [0, 4, 5, 6, 7, ...], in addition to any other
time samples authored in the clip itself. However, we
were seeing extra samples at times [1, 2, 3...] as well.

This bug was caused by the time samples from the clip times
metadata being included in the set of authored time samples
from the clip that were mapped back to stage times. This
mapping process caused the extra samples to appear. The
clip times metadata samples are now added on after the
mapping.

(Internal change: 2069882)

pxr/usd/usd/clip.cpp

@@ -457,28 +457,21 @@ Usd_Clip::GetBracketingTimeSamplesForPath(
     return fetchFromClip || fetchFromAuthoredClipTimes;
 }
 
-std::set<Usd_Clip::InternalTime>
-Usd_Clip::_GetMergedTimeSamplesForPath(const SdfPath& path) const
-{
-    std::set<Usd_Clip::InternalTime> timeSamplesInClip = 
-        _GetLayerForClip()->ListTimeSamplesForPath(_TranslatePathToClip(path));
-    for (const TimeMapping& t : times) {
-        timeSamplesInClip.insert(t.internalTime);
-    }
-
-    return timeSamplesInClip;
-}
-
 size_t
 Usd_Clip::GetNumTimeSamplesForPath(const SdfPath& path) const
 {
-    return _GetMergedTimeSamplesForPath(path).size();
+    // XXX: This is simple but inefficient. However, this function is
+    // currently only used in one corner case in UsdStage, see
+    // _ValueFromClipsMightBeTimeVarying. So for now, we can just
+    // go with this until it becomes a bigger performance concern.
+    return ListTimeSamplesForPath(path).size();
 }
 
 std::set<Usd_Clip::ExternalTime>
 Usd_Clip::ListTimeSamplesForPath(const SdfPath& path) const
 {
-    std::set<InternalTime> timeSamplesInClip = _GetMergedTimeSamplesForPath(path);
+    std::set<InternalTime> timeSamplesInClip = 
+        _GetLayerForClip()->ListTimeSamplesForPath(_TranslatePathToClip(path));
     if (times.empty()) {
         return timeSamplesInClip;
     }
@@ -520,19 +513,10 @@ Usd_Clip::ListTimeSamplesForPath(const SdfPath& path) const
         }
     }
 
-    // If none of the time samples have been mapped, it's because they're
-    // all outside the range of the clip time mappings. In that case, we
-    // apply the same clamping behavior as GetBracketingTimeSamples to
-    // maintain consistency.
-    if (timeSamples.empty()) {
-        for (InternalTime t: timeSamplesInClip) {
-            if (t < times.front().internalTime) {
-                timeSamples.insert(times.front().externalTime);
-            }
-            else if (t > times.back().internalTime) {
-                timeSamples.insert(times.back().externalTime);
-            }
-        }
+    // Each entry in the clip's time mapping is considered a time sample,
+    // so add them in here.
+    for (const TimeMapping& t : times) {
+        timeSamples.insert(t.externalTime);
     }
 
     return timeSamples;

pxr/usd/usd/clip.h

@@ -181,9 +181,6 @@ struct Usd_Clip
 private:
     friend class UsdStage;
 
-    std::set<InternalTime>
-    _GetMergedTimeSamplesForPath(const SdfPath& path) const;
-
     bool 
     _GetBracketingTimeSamplesForPathInternal(const SdfPath& path, 
                                              ExternalTime time, 

pxr/usd/usd/testenv/testUsdValueClips.py

@@ -289,6 +289,18 @@ def test_ClipTiming(self):
         self.CheckTimeSamples(attr)
         self.CheckTimeSamples(attr2)
 
+    def test_ClipTimeSamples(self):
+        """Test that each stage time in a clips time mapping is treated as
+        a time sample."""
+        stage = Usd.Stage.Open('timeSamples/root.usda')
+
+        model = stage.GetPrimAtPath('/Model')
+        attr = model.GetAttribute('size')
+
+        self.assertEqual(attr.GetTimeSamples(),
+                         [0.0, 2.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0])
+        self.CheckTimeSamples(attr)
+
     def test_ClipTimingOutsideRange(self):
         """Tests clip retiming behavior when the mapped clip times are outside
         the range of time samples in the clip"""

pxr/usd/usd/testenv/testUsdValueClips/timeSamples/clip.usda

@@ -0,0 +1,10 @@
+#usda 1.0
+
+def "Model"
+{
+    float size.timeSamples = {
+        0: 0,
+        2: 2.0,
+        4: 4.0
+    }    
+}

pxr/usd/usd/testenv/testUsdValueClips/timeSamples/root.usda

@@ -0,0 +1,25 @@
+#usda 1.0
+
+def "Ref"
+{
+    float size = 1.0
+    float size.timeSamples = {
+        2: -2.0,
+        4: -4.0,
+    }
+}
+
+def "Model" (
+    references = </Ref>
+
+    clips = {
+        dictionary default = {
+            asset[] assetPaths = [@./clip.usda@]
+            string primPath = "/Model"
+            double2[] active = [(0.0, 0)]
+            double2[] times = [(0, 0), (4, 4), (5, 0), (6, 1), (7, 2), (8, 3), (9, 4)]
+        }
+    }
+)
+{
+}