Page MenuHome
Differential D14199

Subdivision node: add input for vertex creases
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Feb 26 2022, 3:15 AM.

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
Branch
vertex_crease_subdiv_node_input (branched from master)
Build Status
Buildable 21844
Build 21844: arc lint + arc unit

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
Kévin Dietrich (kevindietrich) added a comment.Feb 26 2022, 3:17 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
24–30

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
Kévin Dietrich (kevindietrich) updated this revision to Diff 48711.Feb 26 2022, 5:24 AM
  • Add versioning for edge crease input socket.
Harbormaster completed remote builds in B20735: Diff 48711.Feb 26 2022, 5:24 AM
Aaron Carlisle (Blendify) accepted this revision.Feb 26 2022, 5:43 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
Hans Goudey (HooglyBoogly) added a comment.Mar 7 2022, 8:28 PM

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.

Kévin Dietrich (kevindietrich) added a comment.Apr 1 2022, 10:38 AM
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?

Hans Goudey (HooglyBoogly) added a comment.EditedApr 1 2022, 7:40 PM

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
67

Nodes should generally only do a single field evaluation if possible, in case any of the field network is shared between the inputs.

Kévin Dietrich (kevindietrich) added a comment.Apr 27 2022, 2:12 PM
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
67

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)?

Hans Goudey (HooglyBoogly) added a comment.EditedApr 27 2022, 3:19 PM
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
67

Oh right, sorry, I missed that this had to evaluate on two separate domains.

Kévin Dietrich (kevindietrich) updated this revision to Diff 50898.Apr 27 2022, 3:35 PM
  • 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
Hans Goudey (HooglyBoogly) requested changes to this revision.Apr 27 2022, 6:59 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
Kévin Dietrich (kevindietrich) updated this revision to Diff 51115.May 3 2022, 11:15 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
Jacques Lucke (JacquesLucke) accepted this revision.May 4 2022, 3:21 PM

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?


subdiv_crease.blend829 KBDownload

This revision is now accepted and ready to land.May 4 2022, 3:21 PM
Closed by commit rB1a98bec40ec2: Subdivision node: add input for vertex creases (authored by Kévin Dietrich (kevindietrich)). · Explain WhyMay 4 2022, 7:01 PM
This revision was automatically updated to reflect the committed changes.
Kévin Dietrich (kevindietrich) added a commit: rB1a98bec40ec2: Subdivision node: add input for vertex creases.

Revision Contents
Changeset List

PathSize
source/
blender/
blenloader/
intern/
7 lines
nodes/
geometry/
nodes/
95 lines

Diff 50898

View Options

source/blender/blenloader/intern/versioning_300.c

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc

Loading...