VDB AX Houdini SOP UI (Sync AX 7ac2f67)

[richhones]

Following the discussion on the mailing list here, this PR updates the VDB AX Houdini SOP node with the latest changes in the dneg/openvdb_ax repo (7ac2f67).

The main thing holding back the integration of the SOP is the finalising of the UI. The current UI (after some initial changes following discussion on the mailing list) is:

Are we happy with this UI? Or do we need to make further changes such as the 'Create New Attributes/Grids' options becoming a list parameter like in the Attribute Wrangle SOPs?

From my previous message on the mailing list

Things to note:
Changing the 'Create Attributes' toggle to a 'Attributes to Create' list will require changes to the executables as they handle the attribute/volume creation, so I have omitted it in favour of simply renaming the parameter in these first pass of changes. But it might be best to just make this change proper if we aren't in a rush to get the SOP out. [update: this could be kept in the SOP level to avoid changes in the Executable for now]
Similarly any extension of the Voxel Activity/Prune options to Points will require changes to the executable and for the Prune options, moved points use movePoints() which will replaces the tree and deleted points uses deleteFromGroup() which internally calls pruneInactive so don't think Prune is useful for points right now.

On the Voxel Activity parm, I personally like it as it seems somewhat analogous to the VDB Points Group parm (i.e. running on a subset of the data contained in the grid) so haven't removed it, its also nice extra functionality compared to say the Volume Wrangle on VDBs, although I will admit that there aren't huge numbers of immediately obvious use cases. Things I have done with it in the past are to say use the active topology of a grid to represent one thing, whilst the values represent another, but it is definitely an advanced option. To clarify, these options match the ValueIterators directly for On/Off/All, so voxels that exist in the leafs (leaves?) of the VDB with these states will be iterated over. Tiles do not get iterated over at the moment but the executable does have functionality that would allow this in the future.

[Idclip]

This PR should just be focused on discussing the UI - we can improve the CI/directory layout after

[danrbailey]

Thanks @richhones. Looks much better. If it's not too much work, it would be nice to switch to the 'Attributes to Change' list now to avoid a challenging migration later.

At least in this first release, I think we should drop the ability to write to all/active/inactive voxels (and only write to active voxels). I would prioritise functionality to work on tiles - ie run over "volumes (voxels only), volumes (voxels and tiles), points" and it's not totally clear how that would work with being able to write to inactive regions.

[Idclip]

At least in this first release, I think we should drop the ability to write to all/active/inactive voxels (and only write to active voxels)

Would it be okay to simply comment out that option and leave the implementation in place? It would help with migration as we use it internally

[danrbailey]

At least in this first release, I think we should drop the ability to write to all/active/inactive voxels (and only write to active voxels)

Would it be okay to simply comment out that option and leave the implementation in place? It would help with migration as we use it internally

Provided there aren't too many cases where things deviate, I'm happy for you to do something like this until we figure out what to do here:

https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb_houdini/openvdb_houdini/SOP_OpenVDB_Visualize.cc#L189

As I mentioned in a previous review, the important thing is that for both volumes and points, the default should be ValueOnIter. This still needs addressing for points:

https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb_ax/openvdb_ax/compiler/PointExecutable.cc#L398

This is kind of a soft preference here and SideFX probably have final say, but I feel like many of these types of UIs tend to disable parameters rather than hide them? I'm talking specifically about parameters like "VDB Points Group", which currently disappears when you switch mode which moves the snippet window up/down and I personally find a little jarring. I believe there are examples of both, so not sure what the recommended behavior is here...

[Idclip]

As I mentioned in a previous review, the important thing is that for both volumes and points, the default should be ValueOnIter. This still needs addressing for points:

Yes, this is irrelevant to the SOP though. We should make this change (but lets try and focus on the SOP). Currently what's been proposed is:

• Compile guards around activity control
• Attributes to change list

Changes to core functionality that you've mentioned are:

• Point executable runs over active values only (by default).
• Support for running over all active values (tiles) for volumes.

[richhones]

Happy to use compile guards for the the voxel iterator type choices if it is confusing, esp. if we are looking at changing the iteration pattern i.e. to include tiles etc. (more on this later).

Don't think adding the "Attributes to create" list parameter later would be too much of an issue for backwards compatibility (simply need to set either " * " or " " values). I did have a look into adding this using parseNames() (to avoid having custom regex) however this function doesn't match the functionality of the group/list parameters on Attribute Wrangle SOP - i.e. for wildcards only the single character "*" works, possibly some issues with ^ too. I imagine this would be expected to match Houdini pattern matching (allow i.e. padding words with *). Is this function supposed to work in that way? If so, should this function be updated before adding another use of it here? If not, custom regex will be needed to if we want to match the Attribute Wrangle proper (does this need to match anyway?). Similarly, for both the current setup and an Attributes To Create list, do we need to do anything wrt the behaviour for attributes accessed that don't exist and aren't to be created? Attribute Wrangles allow read from these (zero value) but not write. Do we need to match that functionality or are we fine to error as we do now?

Re: voxel and tile iteration - whilst not explicitly a UI thing, we should definitely discuss the Houdini/VolumeExecutable functionality as it is a pretty big change. It sounds like the Houdini SOP should by default act on active tiles as well as active voxels (all active nodes?), is there consensus on whether this is default functionality should actually be propagated to the VolumeExecutable itself? Currently to do this requires repeated runs of executable->execute() setting different tree levels between runs. So, if this is the recommended default, this workflow should be tidied up. Also, assuming we wish this to be configurable in Houdini, any preferences on how we expose the different tree-level combinations: a max depth (voxel) and min depth(internal node, leaf); optional depth toggles; no options - always run on all?

Re: vdb points group, I will defer to whatever is the recommended thing here - although as you say there are examples of both. I personally prefer when the disabling of parameters is more about toggling options within an existing "mode", so parameters that aren't relevant to that aren't shown, i.e. no combination of parameters in Volume mode will expose that parameter so it doesn't need to be visible.

[richhones]

As discussed during TSC Meeting today, things to change here:

• Add Attributes To Create List using UT_String::multiMatch()
• Rather than exposing tree levels, add option to Densify constant tiles (default to on) before execution (this can be optimised later to stream densify rather than do all upfront)
• Wrap voxel iterator in compile guards
• Leave errors in place for missing attributes for now, in future can add ability to use optional attributes with default values like in VEX.

Can continue to discuss other minor details here such as hiding/disabling of VDB Points Group parameter for Volume mode, and any other comments anyone has.

[jmlait]

Yeah, disable vs invisible is something without a clear cut & dried answer. There are some high level rules though.

Reasons to not disable:

• Shows parameters that are irrelevant, making the UI more "scary"
• Creates "gap-tooth" look when you have alternating disabled/enabled parmeters

Reasons to not invisible:

• Twitch parameter pane where things move up and down on you, or suddenly "change" from one parm to another.
• Hides potential functions, making them harder to discover. (Recall the whole fight over hiding menu entries vs disabling) If you see a disabled parm, you know there is a way to enable it, so know that thing is possible at least.

In this case ... hmm... Dan is right that it will cause the snippet pane to jump when the active voxels is removed. On the other hand, is it common to switch those modes? If a node usually "lives" in one configuration or another, the disorientation of the switch is minimal. It is also the case you want the snippet as high in the UI as possible to minimize the need to scroll to get to it, so having that invisible moves it up a line in the vdb-volume case.

[richhones]

I have now pushed up an updated version that includes the Attributes To Create list (using UT_String::multiMatch to match Attribute Wrangle list behaviour), adds a Densify Active Tiles option for grids being written to (and only prunes grids written to also), adds compile guards for the Activity options and moves all extra parameters (prune, densify, compact attributes) to the Options tab.

This looks like the following:

RE: the VDB Points Group option disabling vs hiding, @jmlait I would say that the node is more likely to "live" in either the Points or Volume configuration for its lifetime, it isn't something that users change often when working a placed node so I have chosen hiding here. Similarly the Volume/Point specific options i.e. Densify/Prune (Volumes) and Compact Attributes (Points) are hidden/unhidden within the Options tab depending on the mode.

Let me know your thoughts!

Note: there is plenty of room for optimisation of the densify/prune options but these can come later as they will require changes to the VolumeExecutable. I.e. we can look to densify and prune grids/leafs as they have been iterated over, it currently densifies all written to grids upfront and prunes them all after execution.

[danrbailey]

Looks good to me, thanks @richhones! One minor comment - we don't have any way in native Houdini to "densify" / "voxelize" (not even through HOM as far as I'm aware), so the language of what "densify" means is likely not well understood. Perhaps it would be better to just make this option "ignore active tiles" (default off)? After all, that is essentially what's happening here, the tile is being ignored from the evaluation.

This also provides a smoother path to enable more advanced optimization such as detecting (@density = 1) and simply modifying the tile value directly, rather than densifying and re-constifying the tile. Otherwise, the implementation of how tile values are modified is tied quite directly to the UI.

[richhones]

@danrbailey thanks! I'm fine with invert that parameter and change the name to "Ignore Active Tiles" to allow the optimisation stuff sure. About the terminology "densify", there is the Densify SOP though right? Although not native. Anyway, will make that change.

[richhones]

Before this gets merged I should probably add backwards compatibility for the Ignore tiles option (defaults to off, but old behaviour would be same as on).

[danrbailey]

Looks good to me, thanks @richhones. Is this intended for 8.0 only?

[jmlait]

My bike-shed would be for "Ignore Constant Tiles" rather than "Ignore Active Tiles" as I'm never clear of the Tile vs Leaf distinction :> But this is a minor bike-shed.

The word "Densify" doesn't exist anywhere I know of in Houdini (I objected to the densify sop!) because I really believe the constant tile optimization should be transparent to the end user.

Logic for invisible vs hidden makes sense, I agree with that reasoning and it should then be invisible to keep the code as high as possible.

[Idclip]

Looks good to me, thanks @richhones. Is this intended for 8.0 only?

Are you referring to a potential 7.2.X release? I think we should just stick to 8. This PR has fixed the UI but there is still no CI and the directory layout needs improving. When this PR is approved it will be merged into feature/ax and I will create a final PR with the aforementioned.

[Idclip]

My bike-shed would be for "Ignore Constant Tiles" rather than "Ignore Active Tiles" as I'm never clear of the Tile vs Leaf distinction :> But this is a minor bike-shed.

I will merge this PR later today and bundle this change into the final PR (CI + repo layout)