Skip to content

Conversation

@apradhana
Copy link
Contributor

This PR adds support for one-direction dilation and extension of a field. To achieve this there are a few notable changes to the FastSweeping tool and SOP Extrapolate:

  • Add an enum for sweeping direction (SWEEP_DEFAULT, SWEEP_GREATER_THAN_ISOVALUE, and SWEEEP_LESS_THAN_ISOVALUE). I also add mSweepingDirection as a member function of the FastSweeping class.
  • Add mExtGridInput as an optional member of the FastSweeping class to provide a default voxel value for the SWEEP_GREATER_THAN_ISOVALUE and SWEEP_LESS_THAN_ISOVALUE cases.
  • Bump isInputSdf as a member variable of the FastSweeping class.

On my TODO List regarding FastSweeping that are not addressed by this PR:

  • Add support for mask grid and extension grid input not having the same transform as the fog/SDF grid input.
  • Revisit the TestFastSweeping debug build.

danrbailey and others added 30 commits April 8, 2021 11:15
Signed-off-by: Dan Bailey <[email protected]>
Signed-off-by: Dan Bailey <[email protected]>
Signed-off-by: Dan Bailey <[email protected]>
Rename the label of the expand voxels to match the current Houdini name.

Signed-off-by: jlait <[email protected]>
Added a placeholder file to avoid the pendingchanges directory
from vanishing.

Did not integerate the change notes as we have to bump our version
number first.

Signed-off-by: jlait <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Signed-off-by: Nick Avramoussis <[email protected]>
Ensure Abs() is defined separately for size_t on all platforms that need it,
not just macOS.

Signed-off-by: Brecht Van Lommel <[email protected]>
Copy link
Contributor

@kmuseth kmuseth left a comment

Choose a reason for hiding this comment

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

Please see my comment related to performance. Another much less serious comment is about the terminology used for this feature. "sweep direction" has a very specific meaning in the context of the fast sweeping method. Specifically, in 3D it's the 8 diagonal direction. However, your choice of terminology (cf. enum FastSweepingDirection) seems to suggest that there are other (new?) directions introduced with your changes. I would argue that you're not really changing the sweep directions (there are still the same 8), however your changing the mask or domain in which the 8 sweeps are applied. Long story short I would prefer you rename FastSweepingDirection to FastSweepingDomain :)

if (acc2) {
// There is an assert upstream to check if mExtGridInput exists if mode != SWEEP_DEFAULT
ExtValueT updateExt = acc2->getValue(d1(ijk));
if (mode == FastSweepingDirection::SWEEP_GREATER_THAN_ISOVALUE) {
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 concerned about these two extra branch tests in the inner-most loop of the old Fast Sweeping algorithm. Have you tested the performance impact? Is there no way of extracting those if-statements into three different kernels - at least so the 99% default cause is not negatively impacted by the new (arguably rare) cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a test with extending a Cd grid from a sphere to a box with 215 million voxels. Before this change, the timing is 12.613s. After the change, I get the following timing:

  • SWEEP_ALL_DIRECTION = 12.645s
  • SWEEP_GREATER_THAN_ISOVALUE = 12.929s
  • SWEEP_LESS_THAN_ISOVALUE = 12.652s.

So sweeping in a given direction does make the algorithm go slower by about 2% in this case.

I think we can rid of these two extra branches by making mode as a template and making the SOP to dispatch the correct instantiated template based on the user input. Let me know what you think.

EXTRAPOLATE_TEST1

EXTRAPOLATE_TEST1_OPENGL

EXTRAPOLATE_TEST1_BENCHMARK

Copy link
Contributor

Choose a reason for hiding this comment

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

@apradhana thanks for looking into this - I think 2% is small enough that it shouldn't block this PR (I'll approve it). At some later time it would be great if you could allow for the the two code path to be selected at compile-time (say with a boolean template parameter) so as to avoid the (granted) small overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @kmuseth. I started taking a stab at it, but couldn't finish it on time. That's my homework for the next iteration.

Idclip and others added 14 commits August 31, 2021 12:42
…dinidsonames

Add optional CMake variable to build a subset of the Houdini DSOs
…_ci_fix

Fixed an issue where the Houdini Cache wasn't being saved correctly
Signed-off-by: Dan Bailey <[email protected]>
…2021-08-03

Add TSC meeting notes from 2021-08-03.
…gb_func

Introduced hsvtorgb and rgbtohsv functions to AX
…abs_pendingchanges

Minor update from PR1097 and added pendingchanges
@jmlait
Copy link
Contributor

jmlait commented Sep 17, 2021

The conflict is because you tried to re-use an existing pending_changes which got merged in the mean time, each PR needs its own unique pending_changes file.

@apradhana apradhana merged commit 6a07998 into AcademySoftwareFoundation:master Sep 17, 2021
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.

7 participants