WIP: Sculpt: dynamic topology refactor #104613

Draft
Joseph Eagar wants to merge 333 commits from temp-sculpt-dyntopo into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Updated Notes

  • DynTopo is highly cache bound; depending on how fragmented the mesh has become upwards of 30% of time can be spent simply fetching memory. This led to a number of design decisions:
    • Information that requires walking the mesh is cached in attributes, e.g. vertex valence counts and boundary flags.
    • There is a defragmenting allocator in another branch, temp-sculpt-dyntopo-hive-alloc that tends to improve performance by 10-30%. It was intended for a later PR.
    • The split/collapse edge functions were rewritten to work in-place, instead of destroying and re-creating geometry as it did before. This lessens fragmentation and is important to improve the performance of any defragmentation scheme.
  • I originally wrote split edge to use a pattern-based subdivider as seems to be the trend in the industry. This turned out to be a mistake. My first tests could subdivide quads, but this led to weird floating quad "diamonds" that eventually converged to triangles anyway. In the end I removed the pattern subdivider altogether as it was producing far too much topological noise.
    • That said a global approach like this is probably necessary for very long, skinny edges, though that hadn't been implemented yet.
  • Collapse was rewritten inside the BMesh module due to the need to access a few internal BMesh API functions. It uses a weird template API to provide callbacks for BMLog; I was not happy about this, but the alternative of pre-emptively logging the entire neighborhood around a single edge simply took too much memory. I actually went back and forth between the two approaches several times.
  • I turned BMesh's tool flags into an attribute. This allows using tool flags without rebuilding the entire PBVH. This was more important when more tools were internally implemented with BMesh and used tool flags, but over time some have been converted to Mesh and ways can be found to make this performant (e.g. the face set init operator converts back to Mesh and copies the data to the original BMesh, avoiding a full PBVH rebuild).
  • The core dyntopo code was refactored into a rough C++ form, but much of the code in pbvh_bmesh.cc has not.

Description

This pull request is a refactor of DynTopo. It's a rebase from sculpt-dev to master.

This refactor improves DynTopo in many ways:

  • Attributes are now supported and properly interpolated, e.g. color attributes, uv maps, etc.
  • Boundaries (mesh, face sets, sharp edges, etc) are now properly handled. Hard edge sculpting is now much easier.
  • Topology Rake follows mesh curvature.
  • Performance improvements:
    • Dynamic PBVH rebalancing.
    • Open addressing hash tables.

In addition various bits and pieces of the sculpt API that were recalculated excessively or stored in undo are now stored in attributes:

  • Original data (coords, color, mask, etc). The original undo based implementation did not work for PBVH_BMESH; not only did it require a hash lookup, but it prevented DynTopo from properly interpolating the original data (which added noise to some tools).
  • Mesh and faceset boundaries are now cached in an attribute, .sculpt_boundary_flags, and the API for dealing with boundaries has been improved.
  • Vertex valences (the number of edges around vertices) are now cached in an attribute, .sculpt_valence.
  • A few flags (recalc valence, triangulate faces, etc) are stored in a byte attribute, '.sculpt_flags'.
  • Face areas are stored in .sculpt_face_areas (as a CD_PROP_FLOAT2, they're double-buffered for threading reasons).

Other changes:

  • BMesh's edge collapse function was rewritten. The old one was fairly buggy and was actually not used by many collapse tools (they tend to have their own implementations of it). The new function is much more robust.

The branch was created by forking sculpt-dev and reverting bits back to master. Here's a list of what's done and still needs to be done:

Points of interest

  • BMesh allocates toolflags as a CustomData layer, instead of reallocating all BMVert/BMEdge/BMFace structs.
    This is needed so sculpt operators that internally use a BMesh don't have to do a messy BMesh -> Mesh ->BMesh
    conversion.
  • BMLog is completely re-written in C++.
  • pbvh_bmesh.cc is split into:
    • pbvh_bmesh.cc: logic related to PBVH_BMESH
    • dyntopo.cc: main dyntopo remeshing loop, also subdivision code.
    • dyntopo_collapse.cc: edge collapse code.
    • dyntopo_intern.hh: header.
  • DynTopo now uses a unified min-max heap to avoid having to build the edge queue (and perform the very expensive triangle in sphere tests) twice.
  • DynTopo's main struct, EdgeQueueContext, is in the process of being cleaned up into a more self-contained C++ class.
  • To avoid the long recompilation times of changing BKE_paint.h I added two new files, BKE_sculpt.hh and BKE_dyntopo.hh. I've
    not really moved anything from BKE_paint.h itself yet though (going to do that in master), just added some new stuff.
  • Original vertex data is now stored in attributes (.sculpt_orig_co, .sculpt_orig_no, .sculpt_orig_mask, etc), so they can be properly interpolated. This also fixes bugs with brushes that assume the original coordinates are roughly normal to the current ones, which can lead to noise with autosmooth.
  • All the various kinds of boundaries and boundary corners have been combined into a bitmask (eSculptBoundary and eSculptCorner) and are accessed through three API methods:
    • eSculptBoundary SCULPT_vertex_is_boundary(SculptSession *ss, PBVHVertRef vertex, eSculptBoundary boundary_mask)
    • eSculptCorner SCULPT_vertex_is_corner(SculptSession *ss, PBVHVertRef vertex, eSculptBoundary corner_mask)
    • eSculptBoundary SCULPT_edge_is_boundary(SculptSession *ss, PBVHEdgeRef edge, eSculptBoundary boundary_mask)

Todo List

Stuff to revert

  • Custom default values for CustomDataLayers.
  • Changes to modifier code
  • New brush engine.
  • Brush properties.
  • Experimental bevel-like smoothing code.
  • Icons
  • Experimental sculpt tools
  • SCULPT_TOOL_XXX entries and RNA code for experimental sculpt tools
  • Original bmesh unique ID API.
  • Storage of operator toolflags in customdata layers for BMesh (keep?).
  • Multires fixes.
  • BMTracer code in BM_edge_collapse.
  • pbvh_bmesh_cache_test
  • Changes to python scripts
  • Code needed to compile with clang-cl on windows.

Tasks

  • [ ] Rewrite BLI_table_gset in C++. I actually got this reviewed and approved ~1.5 years ago,
    before the shift into C++ had really gotten started.

  • Split MSculptVert into attributes:

    • Original coordinates and normals
    • Flags (into one CD_PROP_SHORT layer!).
    • Original color
    • Original mask
    • Replace MSculptVert.stroke_id with stroke id api.
  • Replace uses of SmallHash with blender::Map and blender::Set:

    • Replace/rewrite TableGSet with something that uses blender::Set.
    • Replace usages of SmallHash in dyntopo.cc/dyntopo_collapse.cc,pbvh_bmesh.cc.
  • Port minmax heap to C++

  • Cleanup pbvh_bmesh.cc/dyntopo.cc/dyntopo_collapse.cc some more.

  • Simplify PBVHTriBuf system and make it more C++-like.

  • Review external API of BMLog.

  • Fix 3-valence-vert collapse.

  • Add missing brush properties to ui scripts:

    • autosmooth_projection
    • autosmooth_fset_slide
    • dyntopo control properties
  • Implement custom spacing for dyntopo execution.

Ref #103346

### Updated Notes - DynTopo is highly cache bound; depending on how fragmented the mesh has become upwards of 30% of time can be spent simply fetching memory. This led to a number of design decisions: - Information that requires walking the mesh is cached in attributes, e.g. vertex valence counts and boundary flags. - There is a defragmenting allocator in another branch, `temp-sculpt-dyntopo-hive-alloc` that tends to improve performance by 10-30%. It was intended for a later PR. - The split/collapse edge functions were rewritten to work in-place, instead of destroying and re-creating geometry as it did before. This lessens fragmentation and is important to improve the performance of any defragmentation scheme. - I originally wrote split edge to use a pattern-based subdivider as seems to be the trend in the industry. This turned out to be a mistake. My first tests could subdivide quads, but this led to weird floating quad "diamonds" that eventually converged to triangles anyway. In the end I removed the pattern subdivider altogether as it was producing far too much topological noise. - That said a global approach like this is probably necessary for very long, skinny edges, though that hadn't been implemented yet. - Collapse was rewritten inside the BMesh module due to the need to access a few internal BMesh API functions. It uses a weird template API to provide callbacks for BMLog; I was not happy about this, but the alternative of pre-emptively logging the entire neighborhood around a single edge simply took too much memory. I actually went back and forth between the two approaches several times. - I turned BMesh's tool flags into an attribute. This allows using tool flags without rebuilding the entire PBVH. This was more important when more tools were internally implemented with BMesh and used tool flags, but over time some have been converted to Mesh and ways can be found to make this performant (e.g. the face set init operator converts back to Mesh and copies the data to the original BMesh, avoiding a full PBVH rebuild). - The core dyntopo code was refactored into a rough C++ form, but much of the code in `pbvh_bmesh.cc` has not. ### Description This pull request is a refactor of DynTopo. It's a rebase from sculpt-dev to master. This refactor improves DynTopo in many ways: * Attributes are now supported and properly interpolated, e.g. color attributes, uv maps, etc. * Boundaries (mesh, face sets, sharp edges, etc) are now properly handled. Hard edge sculpting is now much easier. * Topology Rake follows mesh curvature. * Performance improvements: - Dynamic PBVH rebalancing. - Open addressing hash tables. In addition various bits and pieces of the sculpt API that were recalculated excessively or stored in undo are now stored in attributes: * Original data (coords, color, mask, etc). The original undo based implementation did not work for PBVH_BMESH; not only did it require a hash lookup, but it prevented DynTopo from properly interpolating the original data (which added noise to some tools). * Mesh and faceset boundaries are now cached in an attribute, `.sculpt_boundary_flags`, and the API for dealing with boundaries has been improved. * Vertex valences (the number of edges around vertices) are now cached in an attribute, `.sculpt_valence.` * A few flags (recalc valence, triangulate faces, etc) are stored in a byte attribute, '.sculpt_flags'. * Face areas are stored in `.sculpt_face_areas` (as a `CD_PROP_FLOAT2`, they're double-buffered for threading reasons). Other changes: * BMesh's edge collapse function was rewritten. The old one was fairly buggy and was actually not used by many collapse tools (they tend to have their own implementations of it). The new function is much more robust. The branch was created by forking sculpt-dev and reverting bits back to master. Here's a list of what's done and still needs to be done: ## Points of interest - `BMesh` allocates toolflags as a `CustomData` layer, instead of reallocating all `BMVert/BMEdge/BMFace` structs. This is needed so sculpt operators that internally use a `BMesh` don't have to do a messy `BMesh` -> `Mesh` ->`BMesh` conversion. - `BMLog` is completely re-written in C++. - `pbvh_bmesh.cc` is split into: - `pbvh_bmesh.cc`: logic related to PBVH_BMESH - `dyntopo.cc`: main dyntopo remeshing loop, also subdivision code. - `dyntopo_collapse.cc`: edge collapse code. - `dyntopo_intern.hh`: header. - DynTopo now uses a unified min-max heap to avoid having to build the edge queue (and perform the very expensive triangle in sphere tests) twice. - DynTopo's main struct, `EdgeQueueContext`, is in the process of being cleaned up into a more self-contained C++ class. - To avoid the long recompilation times of changing `BKE_paint.h` I added two new files, `BKE_sculpt.hh` and `BKE_dyntopo.hh.` I've not really moved anything from `BKE_paint.h` itself yet though (going to do that in master), just added some new stuff. - Original vertex data is now stored in attributes (`.sculpt_orig_co`, `.sculpt_orig_no`, `.sculpt_orig_mask`, etc), so they can be properly interpolated. This also fixes bugs with brushes that assume the original coordinates are roughly normal to the current ones, which can lead to noise with autosmooth. - All the various kinds of boundaries and boundary corners have been combined into a bitmask (eSculptBoundary and eSculptCorner) and are accessed through three API methods: - `eSculptBoundary SCULPT_vertex_is_boundary(SculptSession *ss, PBVHVertRef vertex, eSculptBoundary boundary_mask)` - `eSculptCorner SCULPT_vertex_is_corner(SculptSession *ss, PBVHVertRef vertex, eSculptBoundary corner_mask)` - `eSculptBoundary SCULPT_edge_is_boundary(SculptSession *ss, PBVHEdgeRef edge, eSculptBoundary boundary_mask)` ## Todo List ### Stuff to revert - [x] Custom default values for CustomDataLayers. - [x] Changes to modifier code - [x] New brush engine. - [x] Brush properties. - [x] Experimental bevel-like smoothing code. - [ ] Icons - [x] Experimental sculpt tools - [x] SCULPT_TOOL_XXX entries and RNA code for experimental sculpt tools - [x] Original bmesh unique ID API. - [x] Storage of operator toolflags in customdata layers for BMesh (keep?). - [x] Multires fixes. - [ ] BMTracer code in BM_edge_collapse. - [ ] pbvh_bmesh_cache_test - [x] Changes to python scripts - [ ] Code needed to compile with clang-cl on windows. ### Tasks - [ ] Rewrite BLI_table_gset in C++. I actually got this reviewed and approved ~1.5 years ago, before the shift into C++ had really gotten started. - [x] Split MSculptVert into attributes: - [x] Original coordinates and normals - [x] Flags (into one CD_PROP_SHORT layer!). - [x] Original color - [x] Original mask - [x] Replace MSculptVert.stroke_id with stroke id api. - [x] Replace uses of SmallHash with blender::Map and blender::Set: - [x] Replace/rewrite TableGSet with something that uses blender::Set. - [x] Replace usages of SmallHash in dyntopo.cc/dyntopo_collapse.cc,pbvh_bmesh.cc. - [x] Port minmax heap to C++ - [ ] Cleanup pbvh_bmesh.cc/dyntopo.cc/dyntopo_collapse.cc some more. - [ ] Simplify PBVHTriBuf system and make it more C++-like. - [ ] Review external API of BMLog. - [ ] Fix 3-valence-vert collapse. - [x] Add missing brush properties to ui scripts: - [ ] autosmooth_projection - [ ] autosmooth_fset_slide - [ ] dyntopo control properties - [x] Implement custom spacing for dyntopo execution. Ref #103346
Joseph Eagar added 1 commit 2023-02-11 09:57:18 +01:00
Joseph Eagar added 1 commit 2023-02-11 10:53:22 +01:00
temp-sculpt-dyntopo: Fix compile error
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
3f59466c29
Julien Kaspar added the
Module
Sculpt, Paint & Texture
label 2023-02-11 12:29:12 +01:00
Julien Kaspar changed title from Dyntopo Refactor to #103346 Dyntopo Refactor 2023-02-11 12:30:42 +01:00
Member

Only blender organization members with write access can start builds. See documentation for details.

Only blender organization members with write access can start builds. See [documentation](https://projects.blender.org/infrastructure/blender-bot) for details.
Member

@blender-bot build

@blender-bot build
Joseph Eagar added 1 commit 2023-02-11 20:42:34 +01:00
temp-sculpt-dyntopo: Remove more unrelated changes
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
c5a69cead3
* Removed now-unused SCULPT_TOOL_ entries.
* Removed BLI_even_spline.h.
* Removed some changes to the pose brush.
* Cleaned up some brush flags.
Joseph Eagar added 2 commits 2023-02-11 20:47:48 +01:00
Joseph Eagar added 1 commit 2023-02-11 21:56:44 +01:00
Joseph Eagar added 1 commit 2023-02-11 21:58:07 +01:00
Joseph Eagar added 1 commit 2023-02-11 23:09:32 +01:00
temp-sculpt-dyntopo: Fix layer brush and bmesh conversion bug
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
b413062a09
Joseph Eagar added 1 commit 2023-02-11 23:14:58 +01:00
Joseph Eagar added 1 commit 2023-02-11 23:17:25 +01:00
Joseph Eagar added 1 commit 2023-02-11 23:19:00 +01:00
temp-sculpt-dyntopo: Fix another compiler error
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
d656a1aa13
Joseph Eagar added 1 commit 2023-02-11 23:26:51 +01:00
temp-sculpt-dyntopo: More compiler errors
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
9188bf38c0
Joseph Eagar added 1 commit 2023-02-11 23:34:47 +01:00
And to think, I went through the effort of compiling this
locally with clang
Joseph Eagar added 1 commit 2023-02-11 23:36:25 +01:00
temp-sculpt-dyntopo: And yet more compiler errors
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
8d47a57bae
And to think, I went through the effort of compiling this
locally with clang
Joseph Eagar added 1 commit 2023-02-11 23:44:13 +01:00
temp-sculpt-dyntopo: And more compiler errors. . .
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
ac5c108234
Joseph Eagar added 1 commit 2023-02-11 23:50:10 +01:00
temp-sculpt-dyntopo: Fix compiler error
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
b51ec6b1fc
Joseph Eagar added 1 commit 2023-02-12 00:01:55 +01:00
temp-sculpt-dyntopo: More compiler fixes
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
1c0c0d377c
Joseph Eagar added 1 commit 2023-02-12 00:13:55 +01:00
temp-sculpt-dyntopo: more compiler error fixes
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
b0ba5dece3
Joseph Eagar added 1 commit 2023-02-12 00:19:28 +01:00
temp-sculpt-dyntopo: ANother compiler error
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
555b703afa
Brecht Van Lommel added this to the Sculpt, Paint & Texture project 2023-02-13 09:21:01 +01:00
Joseph Eagar added 5 commits 2023-02-17 12:36:03 +01:00
Joseph Eagar added 2 commits 2023-02-21 03:38:14 +01:00
Julien Kaspar added this to the 3.6 LTS milestone 2023-02-28 19:35:23 +01:00
Joseph Eagar added 5 commits 2023-03-08 03:02:45 +01:00
Joseph Eagar added 1 commit 2023-03-08 13:17:38 +01:00
* Disable idmap debug mode.
* Fixed "show original coordinates" draw mode.
* Fixed bug with idmap debug mode corrupting meshes.
Joseph Eagar added 1 commit 2023-03-09 23:58:45 +01:00
The new BMLog code was failing to call CustomData_bmesh_asan_unpoison
prior to directly memcpy'ing a customdata block.
Joseph Eagar added 1 commit 2023-03-10 04:31:58 +01:00
Joseph Eagar added 1 commit 2023-03-10 10:16:06 +01:00
Joseph Eagar added 2 commits 2023-03-10 11:16:34 +01:00
Also exposed local brush dyntopo disable property in ui.
Also renamed "Dyntopo" panel back to "Dynamic topology."
Brecht Van Lommel changed title from #103346 Dyntopo Refactor to Sculpt: dynamic topology refactor 2023-03-10 13:23:54 +01:00
Joseph Eagar added 2 commits 2023-03-17 00:51:54 +01:00
Joseph Eagar added 1 commit 2023-03-17 01:15:27 +01:00
Joseph Eagar added 1 commit 2023-03-18 23:16:13 +01:00
Joseph Eagar added 1 commit 2023-03-19 01:03:15 +01:00
* Removed experimental uv solver brush code
* Fixed undo bug in draw face sets
* Fixed draw face sets not working in extend mode.
* Fixed extract ops not working in dyntopo.
Joseph Eagar added 2 commits 2023-03-19 11:26:19 +01:00
Joseph Eagar added 1 commit 2023-03-23 01:54:29 +01:00
Joseph Eagar added 1 commit 2023-03-23 02:49:56 +01:00
* Face set projection now works.
* Hard edge mode (which forcibly sets face set projection to 0)
* Exposed settings for both in ui.
Joseph Eagar added 1 commit 2023-03-25 19:55:22 +01:00
Joseph Eagar added 1 commit 2023-03-25 21:48:46 +01:00
PBVH_BMESH no longer tries to use a .hide_poly
layer instead of BM_ELEM_HIDDEN, this is something
that will have to happen inside of the BMesh code
itself.
Joseph Eagar added 1 commit 2023-03-26 23:57:08 +02:00
Joseph Eagar added 1 commit 2023-03-27 00:01:08 +02:00
Joseph Eagar added 1 commit 2023-03-27 01:59:56 +02:00
Joseph Eagar added 2 commits 2023-03-28 12:29:09 +02:00
All DynTopo settings are now stored in a single struct,
DynTopoSettings, which is used by both Brush and Sculpt.

DynTopo settings can be set to either inherit from Sculpt.dyntopo
or Brush.dyntopo; this in controlled with Brush.dyntopo.inherit
which is a bitflag.

This is the original system I came up prior to the more
generic brush property proposals.  I decided to bring it
back for the same reason I wrote it in the first place:
to avoid hardcoding behavior for specific brushes, specifically:

* Simplify always has subdivide and collapse on.
* Snake hook needs a much smaller dyntopo
  spacing than other tools.
temp-sculpt-dyntopo: Fix compile error and minor versioning bug
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
aabc29b249
Joseph Eagar added 1 commit 2023-03-28 13:53:29 +02:00
Joseph Eagar added 1 commit 2023-03-28 14:07:46 +02:00
temp-sculpt-dyntopo: Clean up some more warnings
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
8bbbba537f
Joseph Eagar added 1 commit 2023-03-28 15:40:21 +02:00
Joseph Eagar added 1 commit 2023-03-28 17:01:38 +02:00
Julien Kaspar changed title from Sculpt: dynamic topology refactor to WIP Sculpt: dynamic topology refactor 2023-03-30 10:51:23 +02:00
Julien Kaspar changed title from WIP Sculpt: dynamic topology refactor to WIP: Sculpt: dynamic topology refactor 2023-03-30 10:51:41 +02:00
Member

This is a big change and will need quite a bit of review.
I'll tag @Sergey @ideasman42 and @HooglyBoogly as reviewers for when the patch is in a better state.

I'll test this soon and give some thoughts on the functionality but it's still in a rough state right now.

This is a big change and will need quite a bit of review. I'll tag @Sergey @ideasman42 and @HooglyBoogly as reviewers for when the patch is in a better state. I'll test this soon and give some thoughts on the functionality but it's still in a rough state right now.
Julien Kaspar requested review from Campbell Barton 2023-03-30 10:53:58 +02:00
Julien Kaspar requested review from Sergey Sharybin 2023-03-30 10:53:58 +02:00
Julien Kaspar requested review from Julien Kaspar 2023-03-30 10:53:58 +02:00
Julien Kaspar requested review from Hans Goudey 2023-03-30 10:53:58 +02:00
Member

@JosephEagar I see that a lot of it is still WIP but I was wondering about the stretched topology that is easily caused by stroke speed.
Is this on your agenda to fix it soon?

@JosephEagar I see that a lot of it is still WIP but I was wondering about the stretched topology that is easily caused by stroke speed. Is this on your agenda to fix it soon?
Author
Member

I did fix that, looks like the versioning code for it failed.

I did fix that, looks like the versioning code for it failed.
Joseph Eagar added 2 commits 2023-03-31 08:28:35 +02:00
Joseph Eagar added 1 commit 2023-04-02 23:35:02 +02:00
* TableGSet now uses blender::Map instead of SmallHash.
* Removed a few duplicate CustomData API methods.
* Remove last vestiges of the old mesh element ID
  system.  Note that old files from sculpt-dev with
  CD_MESH_ID layers (which is now CD_DYNTOPO_VERT)
  are detected and patched.
Joseph Eagar added 1 commit 2023-04-03 09:13:38 +02:00
* Expose dyntopo radius scale in UI; needed for snake hook.
* Created a dyntopo "repeat" setting, also used by snake hook.
* Fixed nasty bug in CustomData_bmesh_copy_data_exclude_by_type,
  it may exist in master.
* Dyntopo subdivide now adds to unified minmax heap again.
* Fixed versioning error in Sculpt.dyntopo.
* Fix bug in new blender::Map TableGSet backend.
* Vertex IDs are now only uploaded to the GPU when
  "show ids" overlay is on.
* Dyntopo spacing may now be zero, in which case native
  brush spacing will be used.
* Undo nodes of SCULPT_UNDO_COORDS/COLOR/MASK/FACE_SETS can
  now be pushed multiple times.
* Removed last vestiges of PBVH caching code.
Joseph Eagar added 1 commit 2023-04-06 22:48:40 +02:00
Joseph Eagar added 1 commit 2023-04-06 23:55:50 +02:00
Joseph Eagar added 1 commit 2023-04-09 11:04:45 +02:00
Joseph Eagar added 1 commit 2023-04-09 12:07:51 +02:00
temp-sculpt-dyntopo: Fix bmesh toolflags bug and curvature bug
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
0acc6c51d6
* Fixed a few bugs with handling of CD_TOOLFLAGS layer.
* Sculpt curvature API now handles completely flat geometry.
Julien Kaspar modified the milestone from 3.6 LTS to 4.0 2023-04-12 11:55:36 +02:00
Joseph Eagar added 5 commits 2023-04-14 22:45:27 +02:00
Joseph Eagar added 2 commits 2023-04-18 01:47:24 +02:00
Joseph Eagar added 1 commit 2023-04-19 03:30:26 +02:00
temp-sculpt-dyntopo: bugfixes
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
6d53e23e03
* Fixed crash in color sample (SKEY) tool
* Fixed bugs in visibility code
* Fixed nasty undo bugs when saving full meshes
Joseph Eagar added 1 commit 2023-04-19 08:49:39 +02:00
Joseph Eagar added 1 commit 2023-04-19 08:54:43 +02:00
Joseph Eagar added 1 commit 2023-04-19 08:56:38 +02:00
temp-sculpt-dyntopo: fix compile error
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
4dbd79499a
Joseph Eagar added 1 commit 2023-04-20 01:14:47 +02:00
Joseph Eagar added 2 commits 2023-04-22 01:39:48 +02:00
Joseph Eagar added 1 commit 2023-04-24 20:03:34 +02:00
Joseph Eagar added 1 commit 2023-04-24 20:45:37 +02:00
Joseph Eagar added 1 commit 2023-04-24 20:59:00 +02:00
Joseph Eagar added 1 commit 2023-04-24 21:46:07 +02:00
Joseph Eagar added 3 commits 2023-05-01 22:26:51 +02:00
Joseph Eagar added 5 commits 2023-05-03 01:51:38 +02:00
Note: not all attributes exist all the time.

New attributes:
* flags         : byte
* origco        : float3
* origno        : float3
* origmask      : float
* origcolor     : float4
* curvature_dir : float3;
* valence       : int;
Mostly done, just need to remove MSculptVert struct itself
Joseph Eagar added 1 commit 2023-05-03 07:52:54 +02:00
Joseph Eagar added 4 commits 2023-05-03 09:22:00 +02:00
Joseph Eagar added 1 commit 2023-05-06 00:58:58 +02:00
Member

I've tried testing the latest branch and patch again but there are still significant issues with undo crashes.

Some other thigns to note with an example video:

  • Any brush using dyntopo needs to remesh the surface already on press. Otherwise the results are not as expected
  • The Smooth brush doesn't work properly when dyntopo is enabled (It collapses verts clsoer towards each other)
  • When using Undo, some previously remeshed faces are now only verts & edges anymore (All faces are gone)

2023-05-08 11-52-52.mp4

Once these are fixed, maybe it will be easier to test the branch.

Also:

  • When using Shift to smooth, it will never use dyntopo, even if the smooth brush has "No Dyntopo" disabled

Overall I don't know how much the UI still needs to be reverted so I don't want to give any notes for that right now.

I've tried testing the latest branch and patch again but there are still significant issues with undo crashes. Some other thigns to note with an example video: - Any brush using dyntopo needs to remesh the surface already on press. Otherwise the results are not as expected - The Smooth brush doesn't work properly when dyntopo is enabled (It collapses verts clsoer towards each other) - When using Undo, some previously remeshed faces are now only verts & edges anymore (All faces are gone) [2023-05-08 11-52-52.mp4](/attachments/b17f8f9c-4381-41be-b450-217fbc280c79) Once these are fixed, maybe it will be easier to test the branch. Also: - When using `Shift` to smooth, it will never use dyntopo, even if the smooth brush has "No Dyntopo" disabled Overall I don't know how much the UI still needs to be reverted so I don't want to give any notes for that right now.
Member

@JosephEagar If it helps I at least tested some the easy to find issues:

Crashes

  • Paint brush with "No Dyntopo" disabled
  • Multires Smear
  • Cloth with "No Dyntopo" disabled
  • Trim operators
  • Mask Slice operators

Broken

  • Smooth, Enhance details and sharpen operations/brushes
  • Face Set Expand (causes artefacts, only expands the same face set color and on cancel flood fills the entire mesh in another face set)
  • Face Set operators (for example face set visibility)
  • Draw Face Sets
  • Fairing operators
  • Auto-Masking (except for Topology)

Other

  • Using mesh filter will revert last brush stroke dispalcement (Topology is still the same)
  • Remeshed surfaces creates hard edges in Face Corner Color atrributes
  • Using clay for too long eventually causes huge lag
  • Should find a better name for "No Dyntopo" and only have it in the "Advanced" panel
  • Brushes that won't support dyntopo should not even offer the "No Dyntopo" option (Cloth, Slide/Relax, Pose, Boundary, Multires)
  • Layer brush sometimes produces artefacts on remeshing (But otherwise works fantastically!)
  • No Dyntopo support for Grab (Even though Thumb brush has dyntopo support)
@JosephEagar If it helps I at least tested some the easy to find issues: ## Crashes - Paint brush with "No Dyntopo" disabled - Multires Smear - Cloth with "No Dyntopo" disabled - Trim operators - Mask Slice operators ## Broken - Smooth, Enhance details and sharpen operations/brushes - Face Set Expand (causes artefacts, only expands the same face set color and on cancel flood fills the entire mesh in another face set) - Face Set operators (for example face set visibility) - Draw Face Sets - Fairing operators - Auto-Masking (except for Topology) ## Other - Using mesh filter will revert last brush stroke dispalcement (Topology is still the same) - Remeshed surfaces creates hard edges in Face Corner Color atrributes - Using clay for too long eventually causes huge lag - Should find a better name for "No Dyntopo" and only have it in the "Advanced" panel - Brushes that won't support dyntopo should not even offer the "No Dyntopo" option (Cloth, Slide/Relax, Pose, Boundary, Multires) - Layer brush sometimes produces artefacts on remeshing (But otherwise works fantastically!) - No Dyntopo support for Grab (Even though Thumb brush has dyntopo support)
Joseph Eagar added 2 commits 2023-05-09 06:33:50 +02:00
Joseph Eagar added 1 commit 2023-05-09 06:47:55 +02:00
temp-sculpt-dyntopo: Fix incorrect member initialization in last commit
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
58a41db224
Joseph Eagar added 1 commit 2023-05-09 20:20:59 +02:00
Draw face sets' code has been rewritten to use new
sculpt face API.
Joseph Eagar added 3 commits 2023-05-09 21:17:27 +02:00
Member

Turns out the broken smooth brush behavior that I was mentioning is only present if "Weight By Area" is enabled.
So the setting is broken and impossible to test right now.

image

Turns out the broken smooth brush behavior that I was mentioning is only present if "Weight By Area" is enabled. So the setting is broken and impossible to test right now. ![image](/attachments/3d8862a3-107a-4c6a-9275-e34fd774a6a4)
1.2 MiB
Member

@JosephEagar On the topic of the UI and some recent discussions I made some mockups.
This UI could solve multiple issues.

  • Naming of Dyntopo and Remesh changes to "Surface" and "Volume" remeshing in the UI for clarity
  • Instead of having two popover buttons for "Dynamic Topology" and "Remesh", we merge them into one to save space in the header
  • Instead of using a toggle to turn on Dyntopo, it's a radio button to switch to "Surface Remeshing"
  • In this mockup the checkmark in the header next to "Surface Remeshing" and the checkmark for "Dynamic Remeshing" are the same one. This toggle only turns off the remeshing effect while sculpting without exiting Bmesh. This is then far more accessible and clearer than the "DynTopo" toggle in the current layout.
  • "Manual" detailing mode will become obsolete and can be removed
  • The UI between the two remeshing methods mirror each other more closely for similar options and operations

By default it would be set to "Volume Remeshing" to keep the current behavior.

After this patch is merged I suggest that we add a feature to this UI that this popover button and all options within are grayed out if a multires modifier or shape keys are detected.
Remeshing would destroy vital mesh data. So for those cases remeshing is disabled since the sculpting context is on multires levels or the base mesh.

@JosephEagar On the topic of the UI and some recent discussions I made some mockups. This UI could solve multiple issues. - Naming of Dyntopo and Remesh changes to "Surface" and "Volume" remeshing in the UI for clarity - Instead of having two popover buttons for "Dynamic Topology" and "Remesh", we merge them into one to save space in the header - Instead of using a toggle to turn on Dyntopo, it's a radio button to switch to "Surface Remeshing" - In this mockup the checkmark in the header next to "Surface Remeshing" and the checkmark for "Dynamic Remeshing" are the same one. This toggle only turns off the remeshing effect while sculpting without exiting Bmesh. This is then far more accessible and clearer than the "DynTopo" toggle in the current layout. - "Manual" detailing mode will become obsolete and can be removed - The UI between the two remeshing methods mirror each other more closely for similar options and operations By default it would be set to "Volume Remeshing" to keep the current behavior. After this patch is merged I suggest that we add a feature to this UI that this popover button and all options within are grayed out if a multires modifier or shape keys are detected. Remeshing would destroy vital mesh data. So for those cases remeshing is disabled since the sculpting context is on multires levels or the base mesh.
Joseph Eagar added 1 commit 2023-05-14 20:48:18 +02:00
Joseph Eagar added 1 commit 2023-05-15 01:54:10 +02:00
SculptPMap was a referenced counted wrapper
around a vertex to poly map. It was originally
created to solve a bug that no longer exists.
Joseph Eagar added 1 commit 2023-05-15 02:20:48 +02:00
temp-sculpt-dyntopo: Fix broken boundary flags after draw face sets
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
f4951c51e5
Joseph Eagar added 1 commit 2023-05-15 10:05:13 +02:00
temp-sculpt-dyntopo: Fix various bugs related to area weights and dyntopo
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
ea48ba1f8b
Contributor

@JosephEagar On the topic of the UI and some recent discussions I made some mockups.
This UI could solve multiple issues.

  • Naming of Dyntopo and Remesh changes to "Surface" and "Volume" remeshing in the UI for clarity
  • Instead of having two popover buttons for "Dynamic Topology" and "Remesh", we merge them into one to save space in the header
  • Instead of using a toggle to turn on Dyntopo, it's a radio button to switch to "Surface Remeshing"
  • In this mockup the checkmark in the header next to "Surface Remeshing" and the checkmark for "Dynamic Remeshing" are the same one. This toggle only turns off the remeshing effect while sculpting without exiting Bmesh. This is then far more accessible and clearer than the "DynTopo" toggle in the current layout.
  • "Manual" detailing mode will become obsolete and can be removed
  • The UI between the two remeshing methods mirror each other more closely for similar options and operations

By default it would be set to "Volume Remeshing" to keep the current behavior.

After this patch is merged I suggest that we add a feature to this UI that this popover button and all options within are grayed out if a multires modifier or shape keys are detected.
Remeshing would destroy vital mesh data. So for those cases remeshing is disabled since the sculpting context is on multires levels or the base mesh.

If you're taking user feedback at this stage and place, I like this design very much. But I would just go with Remesh, instead of Remeshing, just saves bit more space and its easier on the tongue. Also what users are already used to.

Having Dynamic Remeshing after two settings also called Remeshing makes it bit confusing, I made this mockup how you can change UI slightly to make it look better and simpler to understand in my opinion.

Also I think Settings subpanel could be just directly in Remesh panel, without need to open another one.

> @JosephEagar On the topic of the UI and some recent discussions I made some mockups. > This UI could solve multiple issues. > > - Naming of Dyntopo and Remesh changes to "Surface" and "Volume" remeshing in the UI for clarity > - Instead of having two popover buttons for "Dynamic Topology" and "Remesh", we merge them into one to save space in the header > - Instead of using a toggle to turn on Dyntopo, it's a radio button to switch to "Surface Remeshing" > - In this mockup the checkmark in the header next to "Surface Remeshing" and the checkmark for "Dynamic Remeshing" are the same one. This toggle only turns off the remeshing effect while sculpting without exiting Bmesh. This is then far more accessible and clearer than the "DynTopo" toggle in the current layout. > - "Manual" detailing mode will become obsolete and can be removed > - The UI between the two remeshing methods mirror each other more closely for similar options and operations > > By default it would be set to "Volume Remeshing" to keep the current behavior. > > After this patch is merged I suggest that we add a feature to this UI that this popover button and all options within are grayed out if a multires modifier or shape keys are detected. > Remeshing would destroy vital mesh data. So for those cases remeshing is disabled since the sculpting context is on multires levels or the base mesh. If you're taking user feedback at this stage and place, I like this design very much. But I would just go with Remesh, instead of Remeshing, just saves bit more space and its easier on the tongue. Also what users are already used to. Having Dynamic Remeshing after two settings also called Remeshing makes it bit confusing, I made this mockup how you can change UI slightly to make it look better and simpler to understand in my opinion. Also I think Settings subpanel could be just directly in Remesh panel, without need to open another one.
Joseph Eagar added 3 commits 2023-05-16 02:38:37 +02:00
This will probably not be user-configurable.  It makes
nicer topology and, more importantly, seems to improve
convergence of subdivide only mode.
temp-sculpt-dyntopo: Fix sculpt boundary corner handling
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
3e29e32c0a
Joseph Eagar added 1 commit 2023-05-16 10:36:33 +02:00
temp-sculpt-dyntopo: Add an option to control hard edge corner pinning
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
f8a77a6265
Member

@Nika-Kutsniashvili I agree that this adjustment to the UI is better.
It also saves a bit more space. Thanks!

To clarify, there would be no more "Remesh" panel. This UI proposal would merge both.

@Nika-Kutsniashvili I agree that this adjustment to the UI is better. It also saves a bit more space. Thanks! To clarify, there would be no more "Remesh" panel. This UI proposal would merge both.
Joseph Eagar added 1 commit 2023-05-16 22:15:10 +02:00
* UVs are now reprojected in dyntopo's topology smoother
  (dyntopo relaxes the topology a small amount to improve
   convergence).
* Moved SCULPT_reproject_cdata to BKE_sculpt_reproject_cdata
  in blenkernel.
* Fixed numerical instability issue that was causing corrupted
  UVs.
Joseph Eagar added 1 commit 2023-05-17 11:07:14 +02:00
* Fixed a few minor smoothing code issues
* Removed a few unused functions
Joseph Eagar added 1 commit 2023-05-17 11:58:18 +02:00
Joseph Eagar added 1 commit 2023-05-17 18:11:58 +02:00
temp-sculpt-dyntopo: Fix various face set bugs
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
5243408f68
* Face set undo now updates boundary flags properly.
* Fixed bug with draw face set brush sometimes only drawing one face set
Joseph Eagar added 1 commit 2023-05-17 19:27:49 +02:00
Joseph Eagar added 2 commits 2023-05-17 23:07:02 +02:00
* Face set edit operator is now supported.
* Fixed various bugs in the visibility operators.
temp-sculpt-dyntopo: Fix undoing past first undo step locking the mesh
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
61fe3ae8d2
Joseph Eagar added 1 commit 2023-05-17 23:13:07 +02:00
temp-sculpt-dyntopo: remove dyntopo smooth shading from UI
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
d25e597adb
Joseph Eagar added 1 commit 2023-05-18 11:11:54 +02:00
Joseph Eagar added 1 commit 2023-05-18 22:57:25 +02:00
* Detects sharp edges by face angles.
* Sharp-angle boundaries are respected by dyntopo only
  for now.
Joseph Eagar added 1 commit 2023-05-19 00:19:39 +02:00
Joseph Eagar added 1 commit 2023-05-19 00:34:19 +02:00
Joseph Eagar added 1 commit 2023-05-19 00:34:49 +02:00
temp-sculpt-dyntopo: Remove debug code from last 2 commits
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
f704e272fc
Joseph Eagar added 1 commit 2023-05-19 09:21:58 +02:00
* ss->bm wasn't being freed at all, a holdover from the
  old PBVH caching experiment I ended up ditching.
* Fixed a few memory leaks in BMLog.
Joseph Eagar added 1 commit 2023-05-19 10:10:35 +02:00
temp-sculpt-dyntopo: Improve dyntopo remesher quality
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
c40d7cc278
* Sharp-by-angle boundaries now test for degenerate triangles.
* Increases maximum number of edges that can be processed at
  once.
Joseph Eagar added 1 commit 2023-05-19 11:02:53 +02:00
temp-sculpt-dyntopo: Fix uv reprojection bug and cleanup code
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
78a0e81d9a
Joseph Eagar added 1 commit 2023-05-19 11:58:29 +02:00
Member

Another thing that needs to be removed from this PR is the "Bend" deformation setting in the pose brush.
This is currently broken and needs its own PR.

Another thing that needs to be removed from this PR is the "Bend" deformation setting in the pose brush. This is currently broken and needs its own PR.
Joseph Eagar added 3 commits 2023-05-22 02:15:25 +02:00
* Move some of the SCULPT_TOOL_DOES_XXX macros to a new file,
  BKE_sculpt.h from DNA_brush_enums.h (they were there because
  rna needs them).
* Fix lazy updating of sharp angle boundaries.
* Detail flood fill now updates the mesh in real time via
  a modal mode and the jobs API.
* Added a quality slider to control dyntopo quality.
temp-sculpt-dyntopo: Cleanup dyntopo code
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
4de7f2b06f
* New header BKE_dyntopo.hh
* blender::dyntopo -> blender::bke::dyntopo
* PBVH_pbvh_bmesh_update_topology ->
  blender::bke::dyntopo::remesh_topology.
* Vertex/tri brush test callbacks are now handled
  via C++ classes.
Joseph Eagar added 1 commit 2023-05-22 03:31:21 +02:00
Author
Member

Another thing that needs to be removed from this PR is the "Bend" deformation setting in the pose brush.
This is currently broken and needs its own PR.

Looks like I forgot to remove the RNA for it, yeesh.

> Another thing that needs to be removed from this PR is the "Bend" deformation setting in the pose brush. > This is currently broken and needs its own PR. Looks like I forgot to remove the RNA for it, yeesh.
Sergey Sharybin reviewed 2023-05-22 16:39:17 +02:00
Sergey Sharybin left a comment
Owner

Just some quick feedback after Julien asked to see if the patch is reviewable.

Just some quick feedback after Julien asked to see if the patch is reviewable.
@ -64,3 +64,3 @@
ATOMIC_INLINE uint64_t atomic_add_and_fetch_uint64(uint64_t *p, uint64_t x)
{
return InterlockedExchangeAdd64((int64_t *)p, (int64_t)x) + x;
return (uint64_t)InterlockedExchangeAdd64((int64_t *)p, (int64_t)x) + x;

Such things are not directly related to the sculpt patch. If it is something needed to solve compilation warning and you still want it in main please move to a separate PR.

Such things are not directly related to the sculpt patch. If it is something needed to solve compilation warning and you still want it in main please move to a separate PR.
Author
Member

It solves an error actually. I'm getting a sign conversion error with MSVC's bundled clang.

It solves an error actually. I'm getting a sign conversion error with MSVC's bundled clang.

Move it to a separate PR, with explanation what error it is fixing and assign appropriate reviewers in the area.

Move it to a separate PR, with explanation what error it is fixing and assign appropriate reviewers in the area.
@ -735,6 +961,12 @@ typedef struct SculptSession {
*/
char needs_flush_to_id;
// id of current stroke, used to detect

Here and in other places, please follow the code style: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

Here and in other places, please follow the code style: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
Author
Member

I'm aware. I've been correcting them as I go along.

I'm aware. I've been correcting them as I go along.
@ -2,2 +2,4 @@
* Copyright 2008 Blender Foundation */
#ifdef __GNUC__
/* I can't even *cast* signed ints in gcc's sign-conversion warning? gcc 10.3.0 -joeedh */

gcc-11 is the minimum.

And not sure if it is something that can be moved to a separate PR (as in, does the warning happen with the existing code already?)

gcc-11 is the minimum. And not sure if it is something that can be moved to a separate PR (as in, does the warning happen with the existing code already?)
@ -4,0 +7,4 @@
#endif
#ifdef __GNUC__
/* I can't even *cast* signed ints in gcc's sign-conversion warning? gcc 10.3.0 -joeedh */

Is it duplicate form above?

Is it duplicate form above?
Author
Member

I'm planning (actually I thought I'd already) to revert smallhash.c. I actually think we should remove it altogether, 'blender::Set' is much better. I've been slowly removing it from the sculpt code.

I'm planning (actually I thought I'd already) to revert smallhash.c. I actually think we should remove it altogether, 'blender::Set' is much better. I've been slowly removing it from the sculpt code.
Joseph Eagar added 5 commits 2023-05-22 23:29:48 +02:00
Joseph Eagar added 2 commits 2023-05-25 12:29:28 +02:00
* remesh_topology has been split into methods in EdgeQueueContext.
  This allows us to run detail flood fill in a special "developer"
  mode to watch the queue update in real time.
temp-sculpt-dyntopo: Improve dyntopo remeshing quality
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
774d08bfd4
Also cleaned up the code some more.
Joseph Eagar added 1 commit 2023-05-26 01:12:22 +02:00
Joseph Eagar added 2 commits 2023-05-29 23:50:14 +02:00
* Remove BMTracer callback system for BM_edge_collapse
* Fix mask/automasking weighting for dyntopo.
* Fix bug in minmax heap
Joseph Eagar added 1 commit 2023-05-30 02:07:11 +02:00
Joseph Eagar added 2 commits 2023-05-30 07:03:24 +02:00
* Certain tools need to have their original coordinates
  attribute processed during autosmoothing and topology
  raking, otherwise they get noise.
* For now it's just draw sharp, other brushes will be tested
  (the layer brush might need it).
* Tried to improve the behavior of topology rake a bit more.
Joseph Eagar added 1 commit 2023-05-30 07:29:46 +02:00
temp-sculpt-dyntopo: Fix bug with layer brush exploding the geometry
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
2a0244cf1a
Joseph Eagar added 1 commit 2023-05-30 21:18:12 +02:00
* Cotangent API
* BMesh vertex disk sorting method.
* Lots of small changes to BMesh.
* Remnants of the original bmesh idmap API.
Joseph Eagar added 2 commits 2023-05-30 22:50:45 +02:00
Joseph Eagar added 1 commit 2023-05-31 00:15:17 +02:00
temp-sculpt-dyntopo: fix compile error
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
309cf9fb82
Joseph Eagar added 1 commit 2023-05-31 00:22:00 +02:00
temp-sculpt-dyntopo: fix msvc error
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
dcfb042cba
Joseph Eagar added 1 commit 2023-05-31 00:48:55 +02:00
temp-sculpt-dyntopo: Fix crash in recent cleanup commits
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
64963b6556
Sergey Sharybin requested changes 2023-05-31 10:38:14 +02:00
Sergey Sharybin left a comment
Owner

Did a quick pass, mainly looking into public API and changes outside of the sculpt/painting code.

There are a lot of changes which do not seem to be related to the sculpt project. It is hard to understand what exactly those changes do, and they need to be split off with proper presentation.

The presentation of this PR could also have some love:

  • First of all, it is not a refactor. Code refactor does not change its external behavior.
  • Having a code related description is great, but there should also be a description of changes on the user level

Btw, did you split off the areas which we've discussed yesterday in the meeting?

Did a quick pass, mainly looking into public API and changes outside of the sculpt/painting code. There are a lot of changes which do not seem to be related to the sculpt project. It is hard to understand what exactly those changes do, and they need to be split off with proper presentation. The presentation of this PR could also have some love: - First of all, it is not a refactor. Code refactor does not change its external behavior. - Having a code related description is great, but there should also be a description of changes on the user level Btw, did you split off the areas which we've discussed yesterday in the meeting?
@ -6851,2 +6851,4 @@
row.prop(overlay, "sculpt_mode_face_sets_opacity", text="Face Sets")
row = layout.row(align=True)
row.prop(overlay, "show_sculpt_ids")

I am not sure how this is useful outside of development/debugging. So to me this should be handled in the similar way how we handle show_active_pixels in Cycles: the option is only available if prefs.experimental.use_cycles_debug and prefs.view.show_developer_ui.

From a quick look, it seems that this is very similar level of debugging as showing indices, so can be using context.preferences.view.show_developer_ui.

I am not sure how this is useful outside of development/debugging. So to me this should be handled in the similar way how we handle `show_active_pixels` in Cycles: the option is only available if `prefs.experimental.use_cycles_debug and prefs.view.show_developer_ui`. From a quick look, it seems that this is very similar level of debugging as showing indices, so can be using `context.preferences.view.show_developer_ui`.
Author
Member

This is a debugging feature, yes, it's not meant for the final PR. Since tt has served its purpose so I'll go ahead and remove it. I'll go through and mark other similar things with comments.

This is a debugging feature, yes, it's not meant for the final PR. Since tt has served its purpose so I'll go ahead and remove it. I'll go through and mark other similar things with comments.
@ -898,0 +949,4 @@
"detail_range"
)
if 0:

Remove dead code.

Remove dead code.
@ -1691,3 +1691,3 @@
}
else {
if (FT_Set_Char_Size(font->face, 0, ft_size, BLF_DPI, BLF_DPI) != FT_Err_Ok) {
if (FT_Set_Char_Size(font->face, 0, (FT_F26Dot6)ft_size, BLF_DPI, BLF_DPI) != FT_Err_Ok) {

If this is relevant, move to a separate PR., explaining what it is for.

If it is similar to fixing some compiler warning/errors to the atomic case you can have a single PR title something like "Fix compilation error/warning for on ."

If this is relevant, move to a separate PR., explaining what it is for. If it is similar to fixing some compiler warning/errors to the atomic case you can have a single PR title something like "Fix compilation error/warning for <compiler> on <platform>."
@ -206,6 +210,19 @@ bool BKE_brush_has_cube_tip(const struct Brush *brush, ePaintMode paint_mode);
/* debugging only */
void BKE_brush_debug_print_state(struct Brush *br);
void BKE_brush_get_dyntopo(struct Brush *brush, struct Sculpt *sd, struct DynTopoSettings *out);

Declared but unused function.

I've only noticed this because I was confused by the signature. Can you go over new API declaration and ensure there is no cases like this?

Declared but unused function. I've only noticed this because I was confused by the signature. Can you go over new API declaration and ensure there is no cases like this?
@ -121,6 +121,8 @@ bool CustomData_has_interp(const struct CustomData *data);
*/
bool CustomData_bmesh_has_free(const struct CustomData *data);
bool CustomData_layout_is_same(const struct CustomData *_a, const struct CustomData *_b);

Do not use underscore prefix.

Do not use underscore prefix.
@ -174,6 +179,10 @@ void CustomData_copy_layout(const struct CustomData *source,
/* BMESH_TODO, not really a public function but readfile.c needs it */
void CustomData_update_typemap(struct CustomData *data);
/* copies all customdata layers without allocating data,
Comment style: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
@ -0,0 +125,4 @@
PBVH_None = 0,
PBVH_Subdivide = 1 << 0,
PBVH_Collapse = 1 << 1,
PBVH_Cleanup = 1 << 2, // dissolve verts surrounded by either 3 or 4 triangles then triangulate

Comment style.

Comment style.
@ -640,1 +716,4 @@
struct BMesh *bm;
struct BMIdMap *bm_idmap;
/* TODO: get rid of these cd_ members and use

Is this planned to happen prior to the merge?

Is this planned to happen prior to the merge?
@ -116,0 +144,4 @@
#ifdef __cplusplus
blender::Map<uintptr_t, int> vertmap;
#else
void *vertmap;

The size of blender::Map is not the same as void*. We should not be declaring a structure which is in a public C API and which will have different size and field offsets for C and C++.

The size of `blender::Map` is not the same as `void*`. We should not be declaring a structure which is in a public C API and which will have different size and field offsets for C and C++.
Author
Member

You're right, yeesh, can't believe I did that. I think I'll hide the whole struct behind __cplusplus, as far as I know there isn't any C code using it anyway.

You're right, yeesh, can't believe I did that. I think I'll hide the whole struct behind `__cplusplus`, as far as I know there isn't any C code using it anyway.
@ -320,0 +400,4 @@
BMLog *BKE_pbvh_get_bm_log(PBVH *pbvh);
/**
checks if original data needs to be updated for v, and if so updates it. Stroke_id

Comment style. Please go over the patch and make sure it follows our style guide.

Comment style. Please go over the patch and make sure it follows our style guide.
@ -691,2 +800,4 @@
((void)0)
#define BKE_pbvh_vertex_to_index(pbvh, v) \
(BKE_pbvh_type(pbvh) == PBVH_BMESH && v.i != -1 ? BM_elem_index_get((BMVert *)(v.i)) : (v.i))

This should be an inlined function instead of a macro. Will make it more readable.

P.S. For the macro being correct it should be (v).i and not v.i.

This should be an inlined function instead of a macro. Will make it more readable. P.S. For the macro being correct it should be `(v).i` and not `v.i`.
@ -818,0 +947,4 @@
int BKE_pbvh_get_node_id(PBVH *pbvh, PBVHNode *node);
void BKE_pbvh_set_flat_vcol_shading(PBVH *pbvh, bool value);
void SCULPT_update_flat_vcol_shading(struct Object *ob, struct Scene *scene);

This is a BKE module, not SCULPT.

This is a BKE module, not SCULPT.
@ -818,0 +1012,4 @@
void BKE_pbvh_get_vert_face_areas(PBVH *pbvh, PBVHVertRef vertex, float *r_areas, int valence);
void BKE_pbvh_set_symmetry(PBVH *pbvh, int symmetry, int boundary_symmetry);
#if 0

Remove dead code.

P.S. Please go over the rest of the patch, there more places where dead code is to be removed.

Remove dead code. P.S. Please go over the rest of the patch, there more places where dead code is to be removed.
@ -818,0 +1117,4 @@
# include <float.h>
# include <math.h>
/* Why is atomic_ops defining near & far macros? */

They are coming from windows.h. I think we should undefine them in the atomic_ops_msvc.h, so that we do not pollute the namespace (similarly to what we already do doe the NOGDI, NOMINMAX).

Not sure if there is a way to tell windows.h not define those in the first place, but undefining them after the #include <windows.h> is always an option.

They are coming from `windows.h`. I think we should undefine them in the `atomic_ops_msvc.h`, so that we do not pollute the namespace (similarly to what we already do doe the NOGDI, NOMINMAX). Not sure if there is a way to tell `windows.h` not define those in the first place, but undefining them after the `#include <windows.h>` is always an option.
@ -770,6 +778,16 @@ if(WITH_OPENVDB)
add_definitions(-DWITH_OPENVDB ${OPENVDB_DEFINITIONS})
endif()
if(WITH_INSTANT_MESHES)

This seems to be unused. There is no instant-meshes library either.

This seems to be unused. There is no `instant-meshes` library either.
@ -56,6 +57,9 @@ const char *no_procedural_access_message = N_(
bool allow_procedural_attribute_access(StringRef attribute_name)
{
if (G.debug_value == 892) {

What is this for?

What is this for?
Author
Member

That's just a debugging thing to make hidden attributes visible in the UI. I'll get rid of it.

That's just a debugging thing to make hidden attributes visible in the UI. I'll get rid of it.
@ -79,0 +95,4 @@
a.layers = b.layers = nullptr;
a.pool = b.pool = nullptr;
if (memcmp((void *)&a, (void *)&b, sizeof(CustomData)) != 0) {

This is very fragile logic. It will already behave strange w.r.t the external data. And in the future if any other runtime field is added it will make this code behave wrong, without a clear indication of it.

This is very fragile logic. It will already behave strange w.r.t the external data. And in the future if any other runtime field is added it will make this code behave wrong, without a clear indication of it.
@ -79,0 +106,4 @@
cla.data = clb.data = nullptr;
cla.default_data = clb.default_data = nullptr;
if (memcmp((void *)&cla, (void *)&clb, sizeof(CustomDataLayer)) != 0) {

Same as above. anonymous_id and implicit_sharing pointers do not affect layout but could lead to layout being wrongly assumed non-matched.

Same as above. `anonymous_id` and `implicit_sharing` pointers do not affect layout but could lead to layout being wrongly assumed non-matched.
@ -60,3 +60,2 @@
#else
# define BLI_assert(a) ((void)0)
# define BLI_assert_msg(a, msg) ((void)0)
# define BLI_assert(a) ((void *)0)

This should be split out.

This should be split out.
@ -18,2 +18,4 @@
* if no arguments are given to the macro, all of pointer
* arguments would be expected to be non-null
*
* ONE-INDEXED!

NOTE: The arguments indices are 1-based.

Also, split out.

`NOTE: The arguments indices are 1-based.` Also, split out.
@ -83,6 +89,17 @@
# define ATTR_ALIGN(x) __attribute__((aligned(x)))
#endif
/* Disable optimization for a function (for debugging use only!)*/

Should also be split out with description when/how to use.

Also, some code in this PR uses ATTR_NO_OPT , even though the comment here says it is for debugging only. Sounds like the production code should never be using this attribute, but it does.

Should also be split out with description when/how to use. Also, some code in this PR uses `ATTR_NO_OPT `, even though the comment here says it is for debugging only. Sounds like the production code should never be using this attribute, but it does.
Author
Member

It's for debugging, yes; sometimes I accidentally leave some in place. This is another place that's supposed to be reverted prior to merging. I actually did submit a separate PR, it was rejected. But I need it for debugging so it stays until then.

It's for debugging, yes; sometimes I accidentally leave some in place. This is another place that's supposed to be reverted prior to merging. I actually did submit a separate PR, it was rejected. But I need it for debugging so it stays until then.
@ -163,3 +163,3 @@
void **BLI_ghash_lookup_p(GHash *gh, const void *key) ATTR_WARN_UNUSED_RESULT;
/**
* Ensure \a key is exists in \a gh.
* Lookup a pointer to the value of \a key in \a gh.

This is a general improvement, which should be moved to main separately.

This is a general improvement, which should be moved to main separately.
@ -339,6 +354,73 @@ BLI_INLINE bool BLI_ghashIterator_done(const GHashIterator *ghi)
typedef struct GSet GSet;
typedef struct TableGSet {

What is this for? When one needs to use it, and when to use GSet?

Get inspiration from comments in primitives like BLI_array.hh.

What is this for? When one needs to use it, and when to use `GSet`? Get inspiration from comments in primitives like `BLI_array.hh`.
Author
Member

It's a hash set with a flat array to speed up iteration. I'm going to rewrite it in C++. I've already been forced to use blender::Map internally as part of my effort to remove all usages of SmallHash from the PR (GHash was not an option, too slow).

It's a hash set with a flat array to speed up iteration. I'm going to rewrite it in C++. I've already been forced to use `blender::Map` internally as part of my effort to remove all usages of `SmallHash` from the PR (`GHash` was not an option, too slow).
@ -0,0 +19,4 @@
//#define BLI_MINMAX_HEAP_VALIDATE
namespace blender {
template<typename Value, typename NodeType> class HeapValueIter {

Same as above.

Same as above.
@ -35,1 +30,3 @@
# pragma warning(error : 4389) /* signed/unsigned mismatch */
/* While regular clang defines __GNUC__ and is handled by the code above, clang-cl does not and
* needs to be handled separately. */
# ifdef __clang__

Spit out.

Spit out.
@ -858,3 +858,3 @@
#define BLI_ENABLE_IF(condition) typename std::enable_if_t<(condition)> * = nullptr
#if defined(_MSC_VER)
#if defined(_MSC_VER) && !defined(__clang__)

Split out.

Split out.
@ -752,6 +752,45 @@ void **BLI_ghash_lookup_p(GHash *gh, const void *key)
return e ? &e->val : NULL;
}
/**

Think there is a description in the header already. No need to duplicate it.

Think there is a description in the header already. No need to duplicate it.
@ -755,0 +778,4 @@
}
/**
* Ensure \a key is exists in \a gh.

Same as above.

Same as above.
@ -4307,6 +4309,159 @@ void blo_do_versions_300(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
if (!DNA_struct_elem_find(fd->filesdna, "Brush", "float", "hard_corner_pin")) {

This should be in versioning_400.c ?

This should be in `versioning_400.c` ?
@ -571,3 +571,3 @@
!cache_settings_equal(csmd) ||
((csmd->rest_source == MOD_CORRECTIVESMOOTH_RESTSOURCE_ORCO) &&
(((ID *)ob->data)->recalc & ID_RECALC_ALL));
(((ID *)ob->data)->recalc & (unsigned int)ID_RECALC_ALL));

This should be moved to separate PR, explaining what it fixed.

This should be moved to separate PR, explaining what it fixed.
@ -29,6 +29,7 @@ set(INC
../makesrna
../render
../windowmanager
../../../intern/atomic

Same as above.

Same as above.
@ -87,6 +87,7 @@ static Mesh *create_ico_sphere_mesh(const int subdivisions,
Mesh *mesh = reinterpret_cast<Mesh *>(BKE_id_new_nomain(ID_ME, nullptr));
BKE_id_material_eval_ensure_default_slot(&mesh->id);
BM_mesh_bm_to_me(nullptr, bm, mesh, &params);

Unrelated change.

Unrelated change.
@ -15,6 +15,7 @@ set(INC
../../render
../../windowmanager
../../../../intern/guardedalloc
../../../../intern/atomic

Here and below: move to separate PR explaining what it fixes.

Here and below: move to separate PR explaining what it fixes.
@ -35,6 +35,7 @@ struct ImBuf;
struct ImageFormatData;
struct Main;
struct MenuType;
struct PBVH;

Not sure why this is needed.

Not sure why this is needed.
@ -27,6 +27,8 @@
#include "BKE_global.h"
#include "BKE_image.h"
#include "BKE_main.h"
#include "BKE_paint.h"

This also seems to be unused.

This also seems to be unused.
@ -2052,1 +2055,3 @@
static void wm_autosave_write(Main *bmain, wmWindowManager *wm)
/* TODO: Move to appropriate headers */
void ED_sculpt_fast_save_bmesh(Object *ob);
struct MemFileUndoStep;

This seems to be a weird use of private API, which needs to have a proper solution.

This seems to be a weird use of private API, which needs to have a proper solution.
Author
Member

I should have explained this more clearly in a comment, sorry; the autosave stuff is actually not part of the PR. The idea was to make testing less painful to users but remove the code prior to merging the final PR.

I should have explained this more clearly in a comment, sorry; the autosave stuff is actually not part of the PR. The idea was to make testing less painful to users but remove the code prior to merging the final PR.
@ -2061,0 +2078,4 @@
ED_sculpt_fast_save_bmesh(ob);
}
else {
multires_flush_sculpt_updates(ob);

This could involve an expensive multires propagation. It is not something you want happen as an auto-save, as it could be causing very intrusive lag before artist makes a stroke.

This could involve an expensive multires propagation. It is not something you want happen as an auto-save, as it could be causing very intrusive lag before artist makes a stroke.
@ -2,6 +2,7 @@
# Copyright 2006 Blender Foundation
set(INC
../../intern/atomic

Combine with other atomic header changes and move to separate PR.

Combine with other atomic header changes and move to separate PR.
Author
Member

I'm not getting errors in atomics on master, currently trying to track down what exactly is causing it.

I'm not getting errors in atomics on master, currently trying to track down what exactly is causing it.
@ -327,3 +328,3 @@
/* free on early-exit */
app_init_data.argv = argv;
app_init_data.argv = (const char **)argv;

This also needs a separate PR.

This also needs a separate PR.
Joseph Eagar added 1 commit 2023-05-31 12:31:23 +02:00
temp-sculpt-dyntopo: Topology rake fixes
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
7235d33e57
* Properly handle smooth boundaries.
Joseph Eagar added 1 commit 2023-05-31 12:50:52 +02:00
temp-sculpt-dyntopo: Fix arm compiler error
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
51e5b7a33b
Joseph Eagar added 3 commits 2023-05-31 14:18:37 +02:00
* Removed the root cause of why they were needed: I had forgotten
to revert changes to BLI_strict_flags.h I apparently made in sculpt-dev.
* Also removed a few other unrelated changes.
* Vcol cell drawing was in this PR for debugging purposes.
  As it is (hopefully) no longer needed for that purpose the
  time has come to remove it.
* Also remove PBVH_BMESH's "smooth shading" and PBVH's "fast draw"
  modes.
Joseph Eagar added 1 commit 2023-05-31 14:21:21 +02:00
Joseph Eagar added 1 commit 2023-05-31 14:23:19 +02:00
Joseph Eagar added 1 commit 2023-06-01 02:09:40 +02:00
* Removed show_ids option
* Cleaned up dyntopo mesh validation code.  It now
  uses BMesh's element checkers.
Joseph Eagar added 2 commits 2023-06-01 02:42:59 +02:00
* No more per-customdata-layer default data, BMIdMap no longer
  needs it.
* Fixed formatting differences.
Joseph Eagar added 1 commit 2023-06-01 05:32:00 +02:00
* Rewrote TableGSet to be fully C++.  Since I'm not sure
  this is generally useful beyond dyntopo I've situated
  it in blenkernel, `BKE_dyntopo_set.hh`.

TODO: Make PBVHNode.bm_unique_verts/bm_other_verts/bm_faces
      members instead of pointers after PBVH->nodes becomes
      a Vector.
Joseph Eagar added 1 commit 2023-06-01 05:33:23 +02:00
Joseph Eagar added 1 commit 2023-06-01 05:34:02 +02:00
Joseph Eagar added 1 commit 2023-06-01 05:41:09 +02:00
Joseph Eagar added 1 commit 2023-06-01 05:42:01 +02:00
Joseph Eagar added 1 commit 2023-06-02 13:29:55 +02:00
temp-sculpt-dyntopo: Fix misc. issues related to UVs
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
1944b90d02
* BKE_sculpt_reproject_cdata is in somewhat of a mess,
  lots of debug code.  It is starting to become more reliable
  though.
* Rewrote the API by which changes in the attribute layout
  of the base mesh is propagated to the sculpt bmesh and
  vice versa.
* The add/remove operators for UV maps, color attributes,
  and normal attributes now properly invoke sculpt undo
  and calls the above API to push the new layer into the
  sculpt mesh.
* Fixed UV island boundary detection.
Joseph Eagar added 1 commit 2023-06-02 13:40:06 +02:00
temp-sculpt-dyntopo: Fix compiler error
Some checks failed
buildbot/vexp-code-experimental-coordinator Build done.
65251574be
Clang is sometimes excessively agressive
in pruning unused template code, and allows
syntax errors.
Joseph Eagar added 1 commit 2023-06-02 13:45:07 +02:00
temp-sculpt-dyntopo: Fix compile error again
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
c49db52349
Joseph Eagar added 1 commit 2023-06-02 13:49:49 +02:00
temp-sculpt-dyntopo: More clang-related compile errors
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
dbebea6343
Joseph Eagar added 1 commit 2023-06-02 13:52:23 +02:00
temp-sculpt-dyntopo: another compiler error
Some checks reported errors
buildbot/vexp-code-experimental-coordinator Build done.
0b5cb2945f
Joseph Eagar added 1 commit 2023-06-02 14:23:22 +02:00
temp-sculpt-dyntopo: Cleanup warnings
All checks were successful
buildbot/vexp-code-experimental-coordinator Build done.
21aeab573d
Joseph Eagar added 1 commit 2023-06-02 14:25:33 +02:00
temp-sculpt-dyntopo: Remove redundant declaration
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
78d338b208
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104613) when ready.
Joseph Eagar added 2 commits 2023-06-03 11:39:18 +02:00
Joseph Eagar added 2 commits 2023-06-03 12:25:29 +02:00
Joseph Eagar added 1 commit 2023-06-04 15:26:08 +02:00
* The new code is a return to the original tracing-based
  callback system for informing PBVH of changes to the mesh.
  This ended up saving quite a lot of memory in undo steps
  and is much faster too.
* Mercifully C++ templates make this much less painful than
  the original C version of tracer callbacks.
* New code is still in somewhat of a wip state.
* Also fixed some bugs in attribute reprojection
Joseph Eagar added 2 commits 2023-06-05 05:49:46 +02:00
This turned out to be a dead end. The original idea of using a
pattern based subdivider was to preserve quads.  But that turned
out to be more trouble than it was worth and just converged to
triangle in the end anyway, except for little floating quads
that messed up smoothing.

So I removed support for quads (quite a while ago) and recent
tests have shown a simple edge splitter is faster and produces
higher quality results.
Joseph Eagar added 1 commit 2023-06-05 06:00:10 +02:00
Joseph Eagar added 1 commit 2023-06-05 07:13:29 +02:00
Joseph Eagar added 1 commit 2023-06-06 11:45:30 +02:00
* Attempted to code a better reproject function, kind of
  turned into a uv smooth funcion with boundary constraints.
  - The idea of the function is you feed it the same weights
    you're using to smooth the vertex coordinates and it derives
    reprojection weights.  But UVs turned out to have so many
    edge cases and other types didn't that it sort of turned
    into a uv-only thing.
* Rewrote edge collapse from scratch yet again, this time
  with almost no use of bitflag tags.  Instead, `blender::Set`
  is used with static buffers.
* Fixed paint tool with corner colors.
* Fixed various UV boundary issues.
Joseph Eagar added 1 commit 2023-06-06 13:00:14 +02:00
Use the new UV smoothing code instead of reprojection for
the small amount of surface relaxation dyntopo does to
reduce numerical instability.
Joseph Eagar added 1 commit 2023-06-06 13:05:07 +02:00
Joseph Eagar added 1 commit 2023-06-06 23:42:34 +02:00
Also fixed very nasty bug in customdata code.