MAYA-105827 - Compute extents for modified GPrims #1112
Conversation
Temporary: Do not trust GPrim extents until we add code to restore the integrity of the data model following an attribute change. See UsdGeomBoundable::GetExtentAttr() for details.
| @@ -540,7 +540,10 @@ void HdVP2Mesh::Sync( | |||
| } | |||
|
|
|||
| if (HdChangeTracker::IsExtentDirty(*dirtyBits, id)) { | |||
| _sharedData.bounds.SetRange(delegate->GetExtent(id)); | |||
| // Do not trust GPrim extents until we add code to restore the integrity of the data model | |||
There was a problem hiding this comment.
Looks good. Please also test the bounding box display mode in the 3d view.
There was a problem hiding this comment.
Maybe better to add a doc here saying VP2 will compute the bounds when none is provided.
There was a problem hiding this comment.
Second thought: bounding box display mode might be broken by this change.
There was a problem hiding this comment.
If it is broken, we can keep storing the extent in the shared data, but each draw item can opt in/out on using it.
There was a problem hiding this comment.
Bounding box was indeed broken (as well as selection when switching to wireframe using wrong BB). This should fix it.
| // If the attribute is not part of the primitive schema, it does not affect extents | ||
| const UsdPrimDefinition& primDefinition = changedPrim.GetPrimDefinition(); | ||
| if (!primDefinition.GetSchemaAttributeSpec(changedPropertyToken)) { | ||
| continue; |
There was a problem hiding this comment.
Filters out the transform notifications when manipulating USD objects.
…t_maya_compute_bounds
lib/mayaUsd/nodes/proxyShapeBase.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| UsdEditContext editContext(stage, stage->GetSessionLayer()); |
There was a problem hiding this comment.
Here is a bit of a problem. The session layer is not serialized together with the root layer.
I would assume that this notification will happen right after the authoring operation and for that, we already have the right edit target set. What's the reason for changing the edit target?
There was a problem hiding this comment.
Yes, this line introduces bugs. I will remove it.
| if (extentsAttr && extentsAttr.HasValue()) { | ||
| VtVec3fArray extent(2); | ||
| if (UsdGeomBoundable::ComputeExtentFromPlugins( | ||
| boundableObj, UsdTimeCode::Default(), &extent)) { |
There was a problem hiding this comment.
Here is another challenging place. What if the extent is animated?
There was a problem hiding this comment.
I must say that reacting by notification is probably not appropriate. There is not enough context to know if the values is/was animated which forces writing a single non-animated extent value. This makes the extent completely wrong if another untouched parameter is kept animated (radius and length of a capsule, for example). There is also no information stored for Undo, and no mechanism to store the undo data of an animated extent (same issue appears in Ufe Undo handler for TypedUsdAttribute::Set).
We could build a set of affected boundables and wait until the Compute function of the shape node is called. At this time we might have a better overall view of the state. We still end up with animation issues: we are expected to compute the extent at the current frame, but might be forced to recompute for the whole clip duration since we have absolutely no idea about the final animated state of the extent, and more specifically, which attributes affect the extent.
|
@JGamache-autodesk I know we are still checking the performance impact, but I took this change for a spin. We still have some work. Compilation fails with USD 20.02 due to change done in the prim (GetPrimDefinition is returning a different type in 20.02) and in sdfpath (GetAsString doesn't exist). It would also be great to add a regression test since we don't have it tested by anything existing. |
…/github.com/Autodesk/maya-usd into t_gamaj/MAYA-105827/let_maya_compute_bounds
The UsdGeomBoundable requirements state that extents must be explicitly authored if any GPrim attribute that affect extents is authored.
This PR tries to keep the extents in sync by recomputing them at notification time.