Light, light filter and camera have incorrect time varying bits for timesampled transform attribute

sdawson-nvidia · October 3, 2023, 1:12am

Hi there. From code analysis, it seems the mentioned types make a call with UsdGeomXformable::IsTransformationAffectedByAttrNamed(attr.GetBaseName()), but this function relies on testing against the xformOp: prefix, and GetBaseName strips any namespaces. For these prims, the DirtyParams bit is also set in timeVaryingBits incorrectly.

See:

github.com

PixarAnimationStudios/OpenUSD/blob/dev/pxr/usdImaging/usdImaging/lightAdapter.cpp#L169


      
          // Determine if the light material network is time varying.
          if (UsdImagingIsHdMaterialNetworkTimeVarying(prim)) {
              *timeVaryingBits |= HdLight::DirtyBits::DirtyResource;
          }
          
          // If any of the light attributes is time varying 
          // we will assume all light params are time-varying.
          const std::vector<UsdAttribute> &attrs = prim.GetAttributes();
          for (UsdAttribute const& attr : attrs) {
              // Don't double-count transform attrs.
              if (UsdGeomXformable::IsTransformationAffectedByAttrNamed(
                      attr.GetBaseName())) {
                  continue;
              }
              if (attr.GetNumTimeSamples()>1){
                  *timeVaryingBits |= HdLight::DirtyBits::DirtyParams;
                  break;
              }
          }
          
          UsdImagingPrimvarDescCache* primvarDescCache = _GetPrimvarDescCache();

github.com

PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/usdGeom/xformable.cpp#L972


      
                  *transform = xform;
                  return true;
              } else {
                  TF_CODING_ERROR("'transform' pointer is NULL.");
              } 
          
              return false;
          }
          
          /* static */
          bool 
          UsdGeomXformable::IsTransformationAffectedByAttrNamed(const TfToken &attrName)
          {
              return attrName == UsdGeomTokens->xformOpOrder ||
                     UsdGeomXformOp::IsXformOp(attrName);
          }
          
          PXR_NAMESPACE_CLOSE_SCOPE

github.com

PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/usdGeom/xformOp.cpp#L200


      
          bool 
          UsdGeomXformOp::IsXformOp(const UsdAttribute &attr)
          {
              if (!attr)
                  return false;
          
              return IsXformOp(attr.GetName());
          }
          
          /* static */
          bool
          UsdGeomXformOp::IsXformOp(const TfToken &attrName) 
          {
              return _IsNamespaced(attrName);
          }
          
          /* static */
          UsdAttribute
          UsdGeomXformOp::_GetXformOpAttr(UsdPrim const& prim, const TfToken &opName,
                                          bool *isInverseOp)
          {

github.com

PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/usdGeom/xformOp.cpp#L92


      
          
              // XXX: backwards compatibility
              (transform)
          
          );
          
          // Validate that the given \p name contains the xform namespace.
          // Does not validate name as a legal property identifier
          static
          bool
          _IsNamespaced(const TfToken& opName)
          {
              return TfStringStartsWith(opName, _tokens->xformOpPrefix);
          }
              
          static
          TfToken
          _MakeNamespaced(const TfToken& name)
          {
              return _IsNamespaced(name) ? name : 
                  TfToken(_tokens->xformOpPrefix.GetString() +

I’ve attached a usd file with lights that have time sampled translations:
lightMotionDirtyBits.usda (10.8 KB)

tomc · October 6, 2023, 10:55pm

Thanks for letting us know! You’re totally correct. The impact of this code is that an animated transform will also cause “DirtyParams” to be set each frame, e.g. over-invalidation, so it will be a performance issue but shouldn’t cause correctness issues. I also found this same pattern in cameraAdapter.cpp and lightFilterAdapter.cpp, FWIW.

Would you be up for filing either a github issue or a github PR for this issue? Thanks!

sdawson-nvidia · October 14, 2023, 4:58am

PR filed: Fix timeVaryingBits for lights, light filters, and cameras for timesampled transform attribute by sdawson-nvidia · Pull Request #2734 · PixarAnimationStudios/OpenUSD · GitHub

Topic		Replies	Views
UsdImaging delegate, odd resync / dirty bits? Hydra	2	174	April 3, 2024
Weird animated material issue when using 60 tc/sec instead of 24 USD	1	116	February 17, 2024
Hydra : updating a transform attribute in Hydra 2.0 Hydra	2	196	January 11, 2024
Passing UsdShadeInput metadata to Hydra Hydra	0	190	September 7, 2023
Accessing the current view matrix and projection matrix in UsdGeomXformable USD beginner	1	210	December 4, 2023

Light, light filter and camera have incorrect time varying bits for timesampled transform attribute

Related topics