[usdGeom] avoid crash from UsdGeomCamera::SetFromCamera

[ablev-nv]

Description of Change(s)

Small change to avoid a crash from UsdGeomCamera::SetFromCamera where EditTarget is set to a weaker layer and a stronger layer on the stage defines the xformOpList on the camera (which results in an invalid UsdGeomXformOp returned from MakeMatrixXform())

New behavior issues a TF_CODING_ERROR instead of a crash.

Also updates the docstring for SetFromCamera.

[jilliene]

Filed as internal issue #USD-6975

[spiffmon]

Hi @ablev-nv !
So actually, the thing you're detecting is a little bit of a false flag, and so the error you're issuing doesn't help as much as it could. I think you can fix this in a more informative way, and for all clients of MakeMatrixXform().

In fact, it doesn't really matter whether a stronger opinion has a transform op in it already or not - we're going to get an unexpected result if a stronger opinion has any set of ops in it, because the AddTransformOp() in the weaker layer will be eclipsed.

You could actually take two different approaches, here, depending on how "smart" we want MakeMatrixXform() to be. The first approach is to say (and document) "all we guarantee is that our authoring will be correct for evaluation at the current EditTarget" This is the more powerful approach, and would not result in any errors in your test case, but:

• it may be surprising behavior (even though via EditTargets it's doing exactly what you've told it to)
• It actually simplifies the implementation of MakeMatrixXform() to a single call to CreateXformOpOrderAttr().Set(ops) for an ops that contains only an "xformOp:transform" and then manually create the transformOp attribute... though you might need to refactor a little code to make this possible.

The other way, which continues to assume we are trying to affect the composed stage, just inserts a check after the ClearXformOpOrder() in MakeMatrixXform() to fetch the opOrder and ensure that it's empty. If it's not, we can reasonably assume that there is a stronger opinion eclipsing the clear we just did, and issue that information as a TF_WARN (since there's no bad code, just a unreconcilable condition from the user).

In either case, SetCamera then need not issue any diagnostic, since any unexpected condition has already been reported, and just return if there's no XformOp for it.

Does that make sense?

[ablev-nv]

Thanks @spiffmon ! That all makes sense.

I was hesitant to change the behavior of MakeMatrixXform like your first proposal, because then MakeMatrixXform would diverge from the other Xformable APIs. But I guess its behavior is already slightly different from the others since it also nukes the xformOpOrder list, so maybe it's not a big deal to diverge further.

That said, I'm inclined to implement your second proposal unless you feel strongly that the first is the best / most correct, in which case I'll give it a shot.

[spiffmon]

I agree, @ablev-nv ; while I believe most USD mutators do possess "the number 1 property" of succeeding regardless of EditTarget even if it means the value you are authoring won't be "seen" on the resolved stage, it would be more confusing to allow one Xformable method to behave that way without bringing them all along. So #2 sound good to me, too!

[asluk]

Hi @spiffmon , I took a pass at modifying the PR per your second proposal while Alan is out. Thanks!

[spiffmon]

Thanks, @asluk - looks good to me! We'll get it queued up.