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

Overhauled OpenVDB Clip SOP camera frustum padding + misc QOL changes #1818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kjyager
Copy link

@kjyager kjyager commented May 16, 2024

The Clip SOPs handling of frustum clipping was odd in several ways, and this caused confusion among users. Padding in particular was opaque and unintuitive.

This change implements more consistent and explainable padding for the camera clipping mode. Padding is specified and applied in normalized device coordinates. Rather than reading the same float3 padding parm used by the other modes, this new implementation adds float2 parms 'padwinx' and 'padwiny', specifying the amount of padding to add on the left/right and top/bottom sides of the camera frustum. These are enabled only in camera mode. Padding is applied symmetrically by default, similar to the old method, but can be made asymmetric by unlinking the parameters. There is no Z-axis padding value, as the near/far plane override parameters make this functionality redundant.

A legacycamclip toggle was added to optionally revert back to the original implementation for compatibility reasons.

To implement this change, it was necessary to make minor modifications to the clipping implementation in the OpenVDB library. A padding arg was added for the camera frustum signature of clip. A similar change was made to the hvdb::drawFrustum() function. All other changes were made just to the SOP itself.

Additional Changes:

  • Tooltips and documentation for the node's parameters have been updated and expanded.
  • Some parameters that were previously non-animatable can now be animated. There was no documented reason given for them being fixed, and I've observed no issues with their being animated. It is the padding (old and new) and near/far clipping plane parameters which have been changed.
  • The xVoxelCount argument of hvdb::frustumTransformFromCamera() is now set to the cameras x-resolution, avoiding the rounding error that previously made the resulting frustum slightly inaccurate.
  • Mask clipping mode now disables the y and z components of the 'padding' parm, to indicate that only uniform padding is supported in that mode.
  • When clipping in mask mode, the warning regarding nonuniform not being supported previously triggered whenever the three 'padding' values were not equal. This caused it to be raised for valid non-zero values as well. I've fixed it to trigger only when the y or z components are non-zero.

The Clip SOPs handling of frustum clipping was odd in several ways, and
this caused confusion among users. Padding in particular was opaque and
unintuitive.

This change implements more consistent and explainable padding for the
camera clipping mode. Padding is specified and applied in normalized
device coordinates. Rather than reading the same float3 padding
parm used by the other modes, this new implementation adds float2 parms
'padwinx' and 'padwiny', specifying the amount of padding to add on the
left/right and top/bottom sides of the camera frustum. These are enabled
only in camera mode. Padding is applied symmetrically by default,
similar to the old method, but can be made asymmetric by unlinking the
parameters. There is no Z-axis padding value, as the near/far plane
override parameters make this functionality redundant.

A `legacycamclip` toggle was added to optionally revert back to the
original implementation for compatibility reasons.

To implement this change, it was necessary to make minor modifications
to the clipping implementation in the OpenVDB library. A padding arg was
added for the camera frustum signature of `clip`. A similar change was
made to the `hvdb::drawFrustum()` function. All other changes were made
just to the SOP itself.

**Additional Changes:**
+ Tooltips and documentation for the node's parameters have been updated
  and expanded.
+ Some parameters that were previously non-animatable can now be
  animated. There was no documented reason given for them being fixed,
  and I've observed no issues with their being animated. It is the
  padding (old and new) and near/far clipping plane parameters which
  have been changed.
+ The `xVoxelCount` argument of `hvdb::frustumTransformFromCamera()` is
  now set to the cameras x-resolution, avoiding the rounding error that
  previously made the resulting frustum slightly inaccurate.
+ Mask clipping mode now disables the y and z components of the
  'padding' parm, to indicate that only uniform padding is supported in
  that mode.
+ When clipping in mask mode, the warning regarding nonuniform not being
  supported previously triggered whenever the three 'padding' values
  were not equal. This caused it to be raised for valid non-zero values
  as well. I've fixed it to trigger only when the y or z components are
  non-zero.

Signed-off-by: Kolton Yager <[email protected]>
Copy link

linux-foundation-easycla bot commented May 16, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kjyager / name: Kole (62f23e2)

@matthewlow-dwa
Copy link
Contributor

/easycla

1 similar comment
@matthewlow-dwa
Copy link
Contributor

/easycla

.setDocumentation(
"Enables legacy camera clipping implementation and controls.\n\n"

"Implementation used prior to (OpenVDB version - / Houdini -). Overestimates "
Copy link
Author

Choose a reason for hiding this comment

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

I've bookmarked the doc-string here so that version numbers can be added when they are known later.

@@ -454,24 +517,36 @@ SOP_OpenVDB_Clip::Cache::getFrustum(OP_Context& context)
}

const bool pad = (0 != evalInt("setpadding", 0, time));
const auto padding = pad ? evalVec3f("padding", time) : openvdb::Vec3f{0};
const bool useLegacyPadding = evalInt("legacycamclip", 0, 0.0f);
Copy link
Author

Choose a reason for hiding this comment

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

Legacy clipping toggle is non-animatable, so evaluating at t = 0 is correct.

@kjyager kjyager marked this pull request as ready for review May 16, 2024 22:52
@kjyager kjyager requested a review from kmuseth as a code owner May 16, 2024 22:52
@kjyager
Copy link
Author

kjyager commented May 16, 2024

Suggesting that Jeff Lait also review

@apradhana apradhana requested a review from jmlait August 13, 2024 20:08
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.

2 participants