Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[al] Check transform values prior to writing to USD. (#993) #246

Conversation

robthebloke
Copy link
Contributor

This PR aims to address two issues when writing data from the Transform nodes to USD (after a TRS tool, or attribute change).

  1. If the incoming transform values match the original values in USD, do not write the values to USD. This is to avoid accidentally inserting overs to layers accidentally (such as the session layer).
  2. Prevent the transform values being written to UsdTimeCode::Default, if the attribute in question has time samples. This is to prevent cases where accidentally writing a default value to the session layer (for example), would create an over that hides all of the underlying animation data.

@robthebloke robthebloke changed the title PIPE-1497 - check transform values prior to writing to USD. (#993) Check transform values prior to writing to USD. (#993) Feb 4, 2020
@robthebloke robthebloke changed the title Check transform values prior to writing to USD. (#993) [al] Check transform values prior to writing to USD. (#993) Feb 4, 2020
Copy link
Contributor

@fowlertADSK fowlertADSK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than possibly changing the utility function to save some lines of code, it looks good.

@@ -43,6 +43,21 @@ MPxTransformationMatrix* TransformationMatrix::creator()
return new TransformationMatrix;
}

//----------------------------------------------------------------------------------------------------------------------
bool hasEmptyDefaultValue(const UsdGeomXformOp& op, UsdTimeCode time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the actual c++ rule but I always prefer to be explicit and wrap these local functions in an unnamed namespace{}.

Was also thinking that we could just make hasEmptyDefaultValue do everything you have below. It's only used in that exact same chunk of code in the 5 or 6 places. Why not just have it also check "timeCode.IsDefault() && op.GetNumTimeSamples()"

Rob Bateman added 2 commits February 12, 2020 10:40
…#993)

* check the TM values prior to writing the data back into USD to prevent duplicate values being written

* remove stupid values

* added a check to prevent writing of default value if no value was previously specified, and keyframes exist

* feedback from code review
@robthebloke robthebloke force-pushed the AL993-check_transform_values_prior_to_writing_to_USD branch from e709662 to c2ccc0d Compare February 12, 2020 01:14
@kxl-adsk kxl-adsk merged commit 00f4e95 into Autodesk:dev Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants