Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)
Aaron Carlisle (Blendify)

Commits

rB1a98bec40ec2: Subdivision node: add input for vertex creases

Summary

This adds an input to the Subdivision node to specify a field to use
for controling vertex creases. Common code with edge creasing was
extracted into utility functions to avoid redundancy.

Diff Detail

Repository: rB Blender

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Feb 26 2022, 3:15 AM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B20734: Diff 48710.Feb 26 2022, 3:15 AM

Here is a simple test file using a randomized attribute to generate vertex creases:

subdiv_node_vertex_crease_input.blend808 KBDownload

Aaron Carlisle (Blendify) requested changes to this revision.Feb 26 2022, 5:08 AM

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).

Aaron Carlisle (Blendify) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
26–32	This change will require versioning to maintain compatibility with older files. See usage of `version_node_input_socket_name()`

This revision now requires changes to proceed.Feb 26 2022, 5:08 AM

Add versioning for edge crease input socket.

Harbormaster completed remote builds in B20735: Diff 48711.Feb 26 2022, 5:24 AM

Fine from my POV but will let Hans comment on anything specific to the geometry nodes.

This revision is now accepted and ready to land.Feb 26 2022, 5:43 AM

Hans Goudey (HooglyBoogly) added 2 blocking reviewer(s): Jacques Lucke (JacquesLucke), Hans Goudey (HooglyBoogly).Feb 28 2022, 9:12 PM

This revision now requires review to proceed.Feb 28 2022, 9:12 PM

Kévin Dietrich (kevindietrich) added a project: Geometry Nodes.Mar 4 2022, 8:42 AM

Adding an input to the node seems very useful. Personally I would be ready to accept this if it didn't depend on the patch that exposed vertex_crease as a builtin attribute (this could just set CD_CREASE directly).
I think there are still unresolved questions with that, and I'm still not convinced it should be a builtin attribute.

In D14199#381285, @Hans Goudey (HooglyBoogly) wrote:

Personally I would be ready to accept this if it didn't depend on the patch that exposed vertex_crease as a builtin attribute (this could just set CD_CREASE directly).

Do you mean this patch should set CD_CREASE directly or the other one?

Basically I'd suggest dropping the other patch for now and relying on improvements to generic attribute editing in edit mode for that part.
This patch could evaluate its vertex crease input field and use something like this logic to write to the CD_CREASE layer:

if (!(vertex_creases.is_single() && vertex_creases.get_internal_single() == 0.0f)) {
  vertex_creases.materialize("CD_CREASE span")

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
69	Nodes should generally only do a single field evaluation if possible, in case any of the field network is shared between the inputs.

In D14199#389719, @Hans Goudey (HooglyBoogly) wrote:
Basically I'd suggest dropping the other patch for now and relying on improvements to generic attribute editing in edit mode for that part.
This patch could evaluate its vertex crease input field and use something like this logic to write to the CD_CREASE layer:
if (!(vertex_creases.is_single() && vertex_creases.get_internal_single() == 0.0f)) {
  vertex_creases.materialize("CD_CREASE span")

This would require using the ID attribute API as the attribute providers could not then create a CD_CREASE layer.

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
69	Is this an information for other future patches, or a requested change? For the latter, how would it work if the number of vertices and do not match (which is most likely)?

In D14199#397240, @Kévin Dietrich (kevindietrich) wrote:

This would require using the ID attribute API as the attribute providers could not then create a CD_CREASE layer.

I think that's okay here, since it's really just meant as an input to the subdivision surface modifier.

Though actually I think it would use the CustomData API, since the CD_CREASE layer isn't a named "generic" attribute.

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
69	Oh right, sorry, I missed that this had to evaluate on two separate domains.

Use ID attribute for vertex creases, remove dependency on D14038.

Harbormaster completed remote builds in B21844: Diff 50898.Apr 27 2022, 3:35 PM

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)Apr 27 2022, 3:36 PM

I think this works, but I'd like to propose some changes just from a code simplicity perspective. I made some changes in P2910.

Don't use the same function for edge and vertex creases, since they don't really share much logic except for clamping.
Multithread writing and clamping of creases.
Avoid retrieving write access if not necessary, to avoid duplicating mesh data.
Use CustomData API instead of ID attribute API, since vertex creases aren't a generic layer.
Slightly simplify code / avoid redundant logic.

Let me know if the diff in that paste looks okay to you.

This revision now requires changes to proceed.Apr 27 2022, 6:59 PM

Apply changes from Hans' patch. I don't see any issues with them
and everything seems to work fine.

Harbormaster completed remote builds in B21949: Diff 51115.May 3 2022, 11:15 PM

Hans Goudey (HooglyBoogly) accepted this revision.May 4 2022, 9:49 AM

LGTM.
General question: is it expected that the subdivided mesh changes a little bit suddenly when the crease is set to 0 compared to e.g. 0.003?