Page MenuHome

Subdivision: add support for vertex creasing
ClosedPublic

Authored by Kévin Dietrich (kevindietrich) on Jan 19 2021, 12:11 PM.
Tags
None
Tokens
"Love" token, awarded by Bit."Like" token, awarded by 1D_Inc."Like" token, awarded by Fracture128."Love" token, awarded by RC12."Love" token, awarded by franMarz."Love" token, awarded by gilberto_rodrigues."Love" token, awarded by silex."Love" token, awarded by EAW."Love" token, awarded by ace_dragon."Like" token, awarded by APEC."Like" token, awarded by MJunk.

Details

Summary

This adds vertex creasing support for OpenSubDiv for modeling, rendering,
Alembic I/O, and USD import.

For modeling, vertex creasing follows the edge creasing implementation with an
operator accessible through the Vertex menu in Edit Mode, and some parameter in
the properties panel. The option in the Subsurf and Multires to use edge
creasing also now affects vertex creasing.

The vertex crease data is stored as a CustomData layer, unlike edge creases
which for now are stored in MEdge, but will in the future also be moved to
a CustomData layer. See comments for details on the difference in behavior
for the CD_CREASE layer between egdes and vertices.

For Cycles this adds a couple of sockets on the Mesh node to hold data about
which vertices are creased (one socket for the indices, one for the weigths).

Viewport rendering of vertex creasing is done by mixing the selected color with
the edge crease color so the creasing is shown to emanate from the vertices,
along the edges. To support rendering vertex and edge creases differently,
EditLoopData now uses ushorts instead of uchars in order to store more
bit flags.

For Alembic and USD, vertex crease support follows the edge crease
implementation, they are always read, but only exported if a Subsurf modifier
is present on the Mesh.

Diff Detail

Repository
rB Blender
Branch
subdiv01_vertex_crease_D10145 (branched from master)
Build Status
Buildable 15608
Build 15608: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Review patch based on Sergey's initial notes.

Removed flags for treating vertex and egde creases differently.

Use a CustomData layer for storing vertex creasing. No changes were made for the
edge creases storage, but if this is the way to go, I can work on a patch for
storing edge creases as CD layers as well.
One challenge I had was making sure vertex creases are properly written and read
from the .blend files. CD_CREASE is also an exceptional case when reading and
riting custom data layers though (I am not sure why because I don't know why the
other expectional layers are also exceptional, but that made it work).

Add vertex sharpness accessor in multires_reshape_smooth.c. I got initially
confused with how the feature is supposed to work. I think I got it in the end,
let me know if something looks obviously wrong. Main thing to note is that I had
to pass the index of the original coarse vertex around the various subdivision
callbacks in order to set the sharpness on the right grid vertex.

The Cycles side changes look good to me. Adding Sergey as blocking reviewer so the overall patch status is not accepted.

Sergey Sharybin (sergey) requested changes to this revision.Jun 1 2021, 9:30 AM

Couple of simple notes in the subdiv code.

Mainly wondering whether there needs to be some sort of mapping of vertex crease similar to the edge one: the edge crease was feeling too sensitive at the lower ranges and not sensitive at the higher values range. Additionally, I believe an infinitely sharp vertex in OpenSubdiv will have sharpness of 10, while in Blender infinitely sharp is 1. So, either I'm missing something, or mapping similar to BKE_subdiv_edge_crease_to_sharpness_{f, char} is needed.

The rest of the changes seems reasonable, but those are from some areas I do not maintain.

source/blender/blenkernel/intern/multires_reshape_smooth.c
514–516

Move the check to an inlinable function is_crease_supported or something similar.

633

Avoid per-vertex call to CustomData_get_layer(), instead get the pointer once and store it in the MultiresReshapeContext.

This revision now requires changes to proceed.Jun 1 2021, 9:30 AM
Kévin Dietrich (kevindietrich) marked 2 inline comments as done.Jun 1 2021, 5:37 PM

Mainly wondering whether there needs to be some sort of mapping of vertex crease similar to the edge one: the edge crease was feeling too sensitive at the lower ranges and not sensitive at the higher values range. Additionally, I believe an infinitely sharp vertex in OpenSubdiv will have sharpness of 10, while in Blender infinitely sharp is 1. So, either I'm missing something, or mapping similar to BKE_subdiv_edge_crease_to_sharpness_{f, char} is needed.

No, you are correct, this is somehow missing, although the Cycles code is remapping to 0-10.

What do you think of renaming the various *edge_crease* functions to remove edge, e.g. BKE_subdiv_edge_crease_to_sharpness_{f, char} -> BKE_subdiv_crease_to_sharpness_{f, char}? I guess I could have preemptively done it.

What do you think of renaming the various *edge_crease* functions to remove edge, e.g. BKE_subdiv_edge_crease_to_sharpness_{f, char} -> BKE_subdiv_crease_to_sharpness_{f, char}? I guess I could have preemptively done it.

This sounds good.
Just verify that the vertex crease control feels good.

  • Store crease layer pointer.
  • Add missing crease to sharpness mapping.
  • Fix viewport data masking.
  • Rename edge_crease functions to be more generic.

Using the same smooth mapping as edge crease also feels a bit better, and allows finer control with low crease values.

The subdiv changes seems fine.

Campbell Barton (campbellbarton) requested changes to this revision.Jun 9 2021, 10:27 AM
  • There is no ability to remove crease data as far as I can see (noted details inline).
  • There are multiple places where the to term crease is used referring to edge_crease, these could be committed as a separate cleanup after this commit, as the result may be noisy, making changes that aren't related to adding vertex creasing.
  • Vertex crease custom data being lost when some modifiers are applied before subdivision-surface (although not in edit-mode). In this test file, toggle edit mode:

    .
  • This patch leaves usage of CD_CREASE in an awkward state.
    • For vertices it's saved into the file.
    • For edges it's run-time data, used in edit-mode, copied to MEdge when exiting edit-mode.

      While this is reasonable given the complications that changing the storage of edge crease would cause. This should be noted since it's not obvious why this is the way it is.

      Suggest to add a paragraph above the CD_CREASE declaration in CustomDataType that explains the current usage of this layer WRT edges/vertices.
intern/cycles/render/mesh.h
216–217

Should be named add_edge_crease.

source/blender/draw/intern/draw_cache_extract_mesh_extractors.c
2760 ↗(On Diff #37829)

Worth commenting this is used for both edge and vertex crease, including what happens when both creases are set.

source/blender/draw/intern/draw_cache_extract_mesh_private.h
84–85

Should be edge_crease_ofs.

source/blender/editors/include/ED_transform.h
57–58

Should name TFM_EDGE_CREASE.

source/blender/editors/space_view3d/view3d_buttons.c
88

The other crease should be named e_crease.

905

Should be scale_e_crease.

source/blender/editors/transform/transform_mode_edge_crease.c
177

Should be initEdgeCrease.

source/blender/makesdna/DNA_mesh_types.h
311

This flag should be exposed in the "Geometry Data" panel, matching edge-crease.

Otherwise there is no way to remove the vertex crease data.

source/blender/makesrna/intern/rna_mesh.c
983–986

This function isn't used.

This revision now requires changes to proceed.Jun 9 2021, 10:27 AM
Kévin Dietrich (kevindietrich) marked 9 inline comments as done.

Adress comments from review.

For rendering the crease in edit mode I decided to separate vertex and edge
creases, while still packing them in EditLoopData.crease (vertex crease uses
the upper 8 bits, while egde crease uses the bottom 8 bits). This way the
current semantics of alpha blending the edge crease color over the edge color
does not break due to conflicts between the two crease types.

Also fixed some rendering issues due to 8-bit data mask used for 16-bit data.

For preserving the vertex crease custom data layer through the modifier stack,
I added some logic to modifierType_Subsurf.requiredDataMask. Not sure if this
is the proper way to go, but the only I found which works.

Accepting as the main concerns I had have been addressed, although there is a compiler warning which could be resolved before committing to master.

source/blender/makesrna/intern/rna_mesh_utils.h:57:15: warning: ‘rna_Mesh_vertex_crease_index_range’ defined but not used [-Wunused-function]
   57 |   static void rna_Mesh_##collection_name##_index_range( \

For preserving the vertex crease custom data layer through the modifier stack,
I added some logic to modifierType_Subsurf.requiredDataMask. Not sure if this
is the proper way to go, but the only I found which works.

This is correct.

Using the Loop Cut tool on a subdivided cube copies the vertex crease from adjacent vertices. Is this desired behaviour?


Personally I would expect Look Cut to just add another edge loop, but otherwise keep the shape of the mesh the same as much as possible (i.e. keep the crease of the new vertices set to zero). I'm not a modeler, though, so I'd also be happy with a confirmation that this is indeed intended behaviour. To me such things shouldn't be left implicit, though.

Apart from this, I've kept the review to the Alembic-specific code, as the rest seems to have been reviewed already by others.

source/blender/io/alembic/intern/abc_reader_mesh.cc
921

Code style.

952

Code style

952–972

This should be moved into its own function. The similar code above could also be moved into its own function in a cleanup commit later.

957

data should get a more descriptive name.

959

can be const

Kévin Dietrich (kevindietrich) marked 4 inline comments as done.Jun 15 2021, 2:28 AM

Using the Loop Cut tool on a subdivided cube copies the vertex crease from adjacent vertices. Is this desired behaviour?

No, this is neither intended nor desired behaviour. The crease is apparently copied at two occasions:

  • once in BM_vert_create via BM_elem_attrs_copy. We can get rid of this by passing CD_MASK_CREASE to BM_elem_attrs_copy_ex, however this will introduce a lot of noise as BM_elem_attrs_copy will need to be modified as well
  • but even if we make use of CD_MASK_CREASE here, the crease is copied a second time through BM_data_interp_from_verts which is called later in BM_edge_split, and we cannot really control what is copied, apart from removing the custom data interpolation function for CD_CREASE which will also affect edge creases (where some interpolation/copy is interesting).

The only solution I found is to reset the vertex crease for the newly created vertex at the end of BM_edge_split, like so:

diff --git a/source/blender/bmesh/intern/bmesh_mods.c b/source/blender/bmesh/intern/bmesh_mods.c
index 76e32667804..3dbdf1a3889 100644
--- a/source/blender/bmesh/intern/bmesh_mods.c
+++ b/source/blender/bmesh/intern/bmesh_mods.c
@@ -700,6 +700,11 @@ BMVert *BM_edge_split(BMesh *bm, BMEdge *e, BMVert *v, BMEdge **r_e, float fac)
     BLI_array_free(oldfaces);
   }
 
+  const int cd_vertex_crease_offset = CustomData_get_offset(&bm->vdata, CD_CREASE);
+  if (cd_vertex_crease_offset != -1) {
+    BM_ELEM_CD_SET_FLOAT(v_new, cd_vertex_crease_offset, 0.0f);
+  }
+
   return v_new;
 }

@Campbell Barton (campbellbarton) what do you think?

While the concern about loopcut is reasonable it would be good to see what kinds of issues users end up running into (with any tool that creates geometry). At the moment it's not clear if each tool should have special handling for vertex-crease, or if this should be handled at a lower level in the API (at least by default).

While the current behavior with loop-cut is not ideal I don't see it as a blocker.

  • Rebase on mater
  • Address Sybren's comments

Fix EditLoopData vertex format, broken after last rebase.

Rebase with master.

Add support for exporting vertex creases to USD.

Clément Foucault (fclem) requested changes to this revision.Jan 3 2022, 1:30 PM

I am not sold on the idea of using the inner wire color as a mean to indicate vertex creasing. It clashes with the selection hint of the edge. The fact that the edge is also half transparent in this mode does not help.
However, showing it around vertices points would only show if vertex selection is enabled. But that's already the case with the current state of the patch.
If you want to go this way, you can look at edit_uv_verts_frag.glsl and how we draw pinned vertices in UV view. The edit mesh view vertices do not use blending so maybe do not use any blend towards the edge.

I don't really like the fact that it's doubling the edit VBO size but I can live with it. The other solution would be to quantize the crease to 4bits only (rounded up). I would argue that the display indicator does not need to be really precise as it is not what you are looking at when tweaking the crease value. I think nobody would spot the difference. Also this would remove the need for the 2 flags.

This revision now requires changes to proceed.Jan 3 2022, 1:30 PM

Added support reading vertex creases from USD. Although the USD importer
does not seem to support reading subdivision data, this makes vertex
creases a fully supported feature as far as all areas are concerned.
(Only export was supported so the patch description is wrong as I mixed
import ans export.)

Use 4 bits to store creases, remove crease flags.

Draw vertex crease on the vertex. We use full crease color if the vertex
is deselected or inactive, with a rather faint outline if the vertex is
selected or active.

I've just looked at the USD/Alembic specific code. LGTM apart from two tiny nags.

source/blender/io/alembic/intern/abc_reader_mesh.cc
839

Flip the condition, return early.

source/blender/io/usd/intern/usd_reader_mesh.cc
523 ↗(On Diff #46549)

const

It should be a custom data layer, not a property which is always stored in vertex.

I also think that it should be represented in UI, like a special Crease section rather than Edges/Vertices separation - when you work with Crease it is preferable to see all available values at once instead of constantly switching between modes with each action to get the full picture of a setup of your selection.
Values can be greyed out according to the current submesh mode (or not).

I would like to suggest to let the same operator (shift+E on default keymap) to control both vertex and edge crease. An option list (or 2 buttons) to choose if we want to affect vertex crease, edge crease or both.
Of course this is out of scope of this patch.

Also I am not sure if interpolating vertex creasing during subdivision is expected behavior.
(screenshot also showing the bigger vertice points)


source/blender/draw/engines/overlay/shaders/edit_mesh_vert.glsl
48

I think we should make the vertex bigger when it has crease. This would make the double color code readable and mimics the edge and uv pinning behavior.

Kévin Dietrich (kevindietrich) marked 4 inline comments as done.Jan 5 2022, 4:24 PM

Also I am not sure if interpolating vertex creasing during subdivision is expected behavior.

That's not really expected (at least I wouldn't expect it). I think to fix this we can remove the CustomData interpolation function (LayerTypeInfo.interp) for CD_CREASE (it is currently reusing the one for bweights). Creases for edges are stored in the edge currently, but even when we move to a custom data layer there as well, interpolation should not be used, but rather copied from coarse edge to corresponding subdivided edge (which is the current behavior, and also what BMesh is doing). So I guess we can remove this.

@Kévin Dietrich (kevindietrich) +1 for not interpolating vertex creases for loop-cut / most subdivision tools. I'd rather this not be done in BM_edge_split though (the custom-data lookup could add unexpected overhead in modifiers for e.g.).

Removing the custom-data interpolation function seems OK, although we might end up wanting it at some point.
Off hand I can't think of any situations it's needed though, so +1 to remove the interpolation function as you suggest.
Code comments should explain why the function isn't present.

  • Cleanup (Sybren's nags).
  • Increase point size if we have a crease.
  • Remove interpolation function for CD_CREASE.
This revision is now accepted and ready to land.Jan 19 2022, 4:06 PM