[usdImaging] fix _GetDirtyBitsForPrimvarChange for PrimvarChangeAdd

pxr/usdImaging/bin/testusdview/CMakeLists.txt

@@ -325,6 +325,11 @@ pxr_install_test_dir(
     DEST testUsdviewVariantSelection
 )
 
+pxr_install_test_dir(
+    SRC testenv/testUsdviewPointWidths
+    DEST testUsdviewPointWidths
+)
+
 
 pxr_register_test(testUsdviewAlive
     PRE_COMMAND "${PYTHON_EXECUTABLE} testUsdviewAliveSetup.py"
@@ -961,6 +966,21 @@ pxr_register_test(testUsdviewVariantSelection
     EXPECTED_RETURN_CODE 0
 )
 
+pxr_register_test(testUsdviewPointWidths
+    PYTHON
+    COMMAND "${CMAKE_INSTALL_PREFIX}/bin/testusdview --testScript testUsdviewPointWidths.py points.usda"
+    IMAGE_DIFF_COMPARE
+        start.png
+        set_point_widths_025.png
+        set_point_widths_050.png
+        set_point_widths_primvar_075.png
+        end.png
+    FAIL 1
+    FAIL_PERCENT 0.002
+    PERCEPTUAL
+    EXPECTED_RETURN_CODE 0
+)
+
 if(OPENCOLORIO_FOUND AND ${PXR_BUILD_OPENCOLORIO_PLUGIN})
     pxr_register_test(testUsdviewColorManagement
         PYTHON

pxr/usdImaging/bin/testusdview/testenv/testUsdviewPointWidths/points.usda

@@ -0,0 +1,8 @@
+#usda 1.0
+(
+    defaultPrim = "World"
+    upAxis = "Y"
+)
+
+def Xform "Scene" {
+}

pxr/usdImaging/bin/testusdview/testenv/testUsdviewPointWidths/testUsdviewPointWidths.py

@@ -0,0 +1,93 @@
+#!/pxrpythonsubst
+#
+# Copyright 2020 Pixar
+#
+# Licensed under the Apache License, Version 2.0 (the "Apache License")
+# with the following modification; you may not use this file except in
+# compliance with the Apache License and the following modification to it:
+# Section 6. Trademarks. is deleted and replaced with:
+#
+# 6. Trademarks. This License does not grant permission to use the trade
+#    names, trademarks, service marks, or product names of the Licensor
+#    and its affiliates, except as required to comply with Section 4(c) of
+#    the License and to reproduce the content of the NOTICE file.
+#
+# You may obtain a copy of the Apache License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the Apache License with the above modification is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the Apache License for the specific
+# language governing permissions and limitations under the Apache License.
+#
+from __future__ import print_function
+
+from pxr.Usdviewq.common import Usd, UsdGeom
+from pxr import Vt, Gf, Sdf
+
+# Remove any unwanted visuals from the view, and enable autoClip
+def _modifySettings(appController):
+    appController._dataModel.viewSettings.showBBoxes = False
+    appController._dataModel.viewSettings.showHUD = False
+    appController._dataModel.viewSettings.autoComputeClippingPlanes = True
+
+def _createPoints(stage):
+    # define the points Prim on the stage
+    points = UsdGeom.Points.Define(stage, Sdf.Path("/Scene/points"))
+
+    positions = []
+    for x in range(-5, 5):
+        positions.append(Gf.Vec3f(x, 0, 10))
+
+    points.CreatePointsAttr().Set(Vt.Vec3fArray(positions))
+    return points
+
+def _createWidths(points, widthSize):
+    # initialize the widths attribute to a specific value
+    widths = []
+    for _ in range(0, len(points.GetPointsAttr().Get())):
+        widths.append(widthSize)
+
+    points.CreateWidthsAttr().Set(widths)
+
+def _createWidthsPrimvar(points, widthSize):
+    widths = []
+    for _ in range(0, len(points.GetPointsAttr().Get())):
+        widths.append(widthSize)
+
+    api = UsdGeom.PrimvarsAPI(points)
+    api.CreatePrimvar(UsdGeom.Tokens.widths, Sdf.ValueTypeNames.FloatArray).Set(widths)
+
+def _removeWidthsPrimvar(points):
+    api = UsdGeom.PrimvarsAPI(points)
+    api.RemovePrimvar(UsdGeom.Tokens.widths)
+
+def _testPointsWidthsAttr(appController):
+    # create the initial set of points that we will be changing the widths on
+    points = _createPoints(appController._dataModel.stage)
+
+    # take a screen shot of the points with their default widths
+    appController._takeShot("start.png")
+
+    # now author the widths attribute to a non-default width size
+    _createWidths(points, 0.25)
+    appController._takeShot("set_point_widths_025.png")
+
+    # change widths to a different value
+    _createWidths(points, 0.5)
+    appController._takeShot("set_point_widths_050.png")
+
+    # now author the widths primvar to override the widths attr
+    _createWidthsPrimvar(points, 0.75)
+    appController._takeShot("set_point_widths_primvar_075.png")
+
+    # remove the widths primvar to revert back to the widths attr
+    _removeWidthsPrimvar(points)
+    appController._takeShot("end.png")
+
+# This test adds and removes primvars that are used/unused by the material.
+def testUsdviewInputFunction(appController):
+    _modifySettings(appController)
+    _testPointsWidthsAttr(appController)

pxr/usdImaging/usdImaging/primAdapter.cpp

@@ -852,53 +852,11 @@ UsdImagingPrimAdapter::_ComputeAndMergePrimvar(
 
 namespace {
 
-// The types of primvar changes expected
-enum PrimvarChange {
-    PrimvarChangeValue,
-    PrimvarChangeAdd,
-    PrimvarChangeRemove,
-    PrimvarChangeDesc
-};
-
-// Maps the primvar changes (above) to the dirty bit that needs to be set.
+// Figure out what changed about the primvar and update the primvar descriptors
+// if necessary
 /*static*/
-HdDirtyBits
-_GetDirtyBitsForPrimvarChange(
-    PrimvarChange changeType,
-    HdDirtyBits valueChangeDirtyBit)
-{
-    HdDirtyBits dirty = HdChangeTracker::Clean;
-
-    switch (changeType) {
-        case PrimvarChangeAdd:
-        case PrimvarChangeRemove:
-        case PrimvarChangeDesc:
-        {
-            // XXX: Once we have a bit for descriptor changes, we should use
-            // that instead.
-            dirty = HdChangeTracker::DirtyPrimvar;
-            break;
-        }
-        case PrimvarChangeValue:
-        {
-            dirty = valueChangeDirtyBit;
-            break;
-        }
-        default:
-        {
-            TF_CODING_ERROR("Unsupported PrimvarChange %d\n", changeType);
-        }
-    }
-
-    return dirty;
-}
-
-// Figure out what changed about the primvar and returns the appropriate dirty
-// bit.
-/*static*/
-PrimvarChange
+void
 _ProcessPrimvarChange(bool primvarOnPrim,
-                      HdInterpolation primvarInterpOnPrim,
                       TfToken const& primvarName,
                       HdPrimvarDescriptorVector* primvarDescs,
                       SdfPath const& cachePath/*debug*/)
@@ -914,25 +872,14 @@ _ProcessPrimvarChange(bool primvarOnPrim,
     }
     bool primvarInValueCache = primvarIt != primvarDescs->end();
 
-    PrimvarChange changeType = PrimvarChangeValue;
-    if (primvarOnPrim && !primvarInValueCache) {
-        changeType = PrimvarChangeAdd;
-    } else if (!primvarOnPrim && primvarInValueCache) {
-        changeType = PrimvarChangeRemove;
-
+    if (!primvarOnPrim && primvarInValueCache) {
         TF_DEBUG(USDIMAGING_CHANGES).Msg(
             "Removing primvar descriptor %s for cachePath %s.\n",
             primvarIt->name.GetText(), cachePath.GetText());
 
         // Remove the value cache entry.
         primvarDescs->erase(primvarIt);
-
-    } else if (primvarInValueCache && primvarOnPrim &&
-               (primvarIt->interpolation != primvarInterpOnPrim)) {
-        changeType = PrimvarChangeDesc;
     }
-
-    return changeType;
 }
 
 } // anonymous namespace
@@ -944,7 +891,7 @@ UsdImagingPrimAdapter::_ProcessNonPrefixedPrimvarPropertyChange(
         TfToken const& propertyName,
         TfToken const& primvarName,
         HdInterpolation const& primvarInterp,
-        HdDirtyBits valueChangeDirtyBit
+        HdDirtyBits primvarDirtyBit
             /*= HdChangeTracker::DirtyPrimvar*/) const
 {
     // Determine if primvar exists on the prim.
@@ -965,21 +912,20 @@ UsdImagingPrimAdapter::_ProcessNonPrefixedPrimvarPropertyChange(
     }
 
     HdPrimvarDescriptorVector& primvarDescs =
-        _GetPrimvarDescCache()->GetPrimvars(cachePath);  
-
-    PrimvarChange changeType =
-        _ProcessPrimvarChange(primvarOnPrim, primvarInterp,
-                              primvarName, &primvarDescs, cachePath);
+        _GetPrimvarDescCache()->GetPrimvars(cachePath);
+
+    _ProcessPrimvarChange(primvarOnPrim, primvarName,&primvarDescs,
+                          cachePath);
 
-    return _GetDirtyBitsForPrimvarChange(changeType, valueChangeDirtyBit);
+    return primvarDirtyBit;
 }
 
 HdDirtyBits
 UsdImagingPrimAdapter::_ProcessPrefixedPrimvarPropertyChange(
         UsdPrim const& prim,
         SdfPath const& cachePath,
         TfToken const& propertyName,
-        HdDirtyBits valueChangeDirtyBit/*= HdChangeTracker::DirtyPrimvar*/,
+        HdDirtyBits primvarDirtyBit/*= HdChangeTracker::DirtyPrimvar*/,
         bool inherited/*=true*/) const
 {
     // Determine if primvar exists on the prim.
@@ -1007,13 +953,12 @@ UsdImagingPrimAdapter::_ProcessPrefixedPrimvarPropertyChange(
     // Determine if primvar is in the value cache.
     TfToken primvarName = UsdGeomPrimvar::StripPrimvarsName(propertyName);
     HdPrimvarDescriptorVector& primvarDescs =
-        _GetPrimvarDescCache()->GetPrimvars(cachePath);  
-
-    PrimvarChange changeType = _ProcessPrimvarChange(primvarOnPrim,
-                                 hdInterpOnPrim,
-                                 primvarName, &primvarDescs, cachePath);
+        _GetPrimvarDescCache()->GetPrimvars(cachePath);
+
+    _ProcessPrimvarChange(primvarOnPrim, primvarName, &primvarDescs,
+                          cachePath);
 
-    return _GetDirtyBitsForPrimvarChange(changeType, valueChangeDirtyBit);          
+    return primvarDirtyBit;
 }
 
 UsdImaging_CollectionCache&