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
buildbot/vexp-code-patch-coordinator Build done. Details
3f59466c29
temp-sculpt-dyntopo: Fix compile error
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
buildbot/vexp-code-experimental-coordinator Build done. Details
c5a69cead3
temp-sculpt-dyntopo: Remove more unrelated changes
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
b413062a09
temp-sculpt-dyntopo: Fix layer brush and bmesh conversion bug
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
buildbot/vexp-code-experimental-coordinator Build done. Details
d656a1aa13
temp-sculpt-dyntopo: Fix another compiler error
Joseph Eagar added 1 commit 2023-02-11 23:26:51 +01:00
buildbot/vexp-code-experimental-coordinator Build done. Details
9188bf38c0
temp-sculpt-dyntopo: More compiler errors
Joseph Eagar added 1 commit 2023-02-11 23:34:47 +01:00
65bdb7d53e temp-sculpt-dyntopo: And yet more compiler errors
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
buildbot/vexp-code-experimental-coordinator Build done. Details
8d47a57bae
temp-sculpt-dyntopo: And yet more compiler errors
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
buildbot/vexp-code-experimental-coordinator Build done. Details
ac5c108234
temp-sculpt-dyntopo: And more compiler errors. . .
Joseph Eagar added 1 commit 2023-02-11 23:50:10 +01:00
buildbot/vexp-code-experimental-coordinator Build done. Details
b51ec6b1fc
temp-sculpt-dyntopo: Fix compiler error
Joseph Eagar added 1 commit 2023-02-12 00:01:55 +01:00
buildbot/vexp-code-experimental-coordinator Build done. Details
1c0c0d377c
temp-sculpt-dyntopo: More compiler fixes
Joseph Eagar added 1 commit 2023-02-12 00:13:55 +01:00
buildbot/vexp-code-experimental-coordinator Build done. Details
b0ba5dece3
temp-sculpt-dyntopo: more compiler error fixes
Joseph Eagar added 1 commit 2023-02-12 00:19:28 +01:00
buildbot/vexp-code-experimental-coordinator Build done. Details
555b703afa
temp-sculpt-dyntopo: ANother compiler error
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
619c2e1eb9 temp-sculpt-dyntopo: Various fixes
* 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
656c075843 temp-sculpt-dyntopo: Fix symmetrize undo and a few ASAN errors
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
0e582f1cc3 temp-sculpt-dyntopo: Fix "weight by area" for smooth brush
Also exposed local brush dyntopo disable property in ui.
29038867a9 temp-sculpt-dyntopo: Add back global dyntopo disable
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
6a14199f7d temp-sculpt-dyntopo: Code cleanup and bugfixes
* 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
ec76d621f6 temp-sculpt-dyntopo: Fix smooth boundary/corner handling
* 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
1889224bd0 temp-sculpt-dyntopo: Fix visibility bug
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
d2ef2d905c temp-sculpt-dyntopo: Make use of DynTopoSettings again
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.
buildbot/vexp-code-experimental-coordinator Build done. Details
aabc29b249
temp-sculpt-dyntopo: Fix compile error and minor versioning bug
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
buildbot/vexp-code-experimental-coordinator Build done. Details
8bbbba537f
temp-sculpt-dyntopo: Clean up some more warnings
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
f4e9cf982d temp-sculpt-dyntopo: Code cleanups
* 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
212eeabd16 temp-sculpt-dyntopo: Various random fixes
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
0acc6c51d6
temp-sculpt-dyntopo: Fix bmesh toolflags bug and curvature bug
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
6d53e23e03
temp-sculpt-dyntopo: bugfixes
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
4dbd79499a
temp-sculpt-dyntopo: fix compile error
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
82ebcc018e temp-sculpt-dyntopo: Begin splitting MSculptVert into attributes
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;
3c40e6c5d9 temp-sculpt-dyntopo: Continue splitting up MSculptVert
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
Joseph Eagar added 1 commit 2023-05-09 20:20:59 +02:00
4891066754 temp-sculpt-dyntopo: Fix bugs in smoothing code and draw face sets
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
3d4d32c568 temp-sculpt-dyntopo: Remove SculptPMap struct
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
Joseph Eagar added 1 commit 2023-05-15 10:05:13 +02:00
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
45e7875b70 temp-sculpt-dyntopo: Bring back 3/4 pole cleanup option
This will probably not be user-configurable.  It makes
nicer topology and, more importantly, seems to improve
convergence of subdivide only mode.
buildbot/vexp-code-experimental-coordinator Build done. Details
3e29e32c0a
temp-sculpt-dyntopo: Fix sculpt boundary corner handling
Joseph Eagar added 1 commit 2023-05-16 10:36:33 +02:00
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
972df91bc5 temp-sculpt-dyntopo: UV reprojection improvements
* 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
64b21b4d1f temp-sculpt-dyntopo: Minor smoothing fixes and code cleanups
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
5243408f68
temp-sculpt-dyntopo: Fix various face set bugs
* 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
62404b3878 temp-sculpt-dyntopo: Improve dyntopo support for face sets
* Face set edit operator is now supported.
* Fixed various bugs in the visibility operators.
Joseph Eagar added 1 commit 2023-05-17 23:13:07 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
d25e597adb
temp-sculpt-dyntopo: remove dyntopo smooth shading from UI
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
ab46277a5d temp-sculpt-boundary: New boundary type: sharp-angle edges
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
f704e272fc
temp-sculpt-dyntopo: Remove debug code from last 2 commits
Joseph Eagar added 1 commit 2023-05-19 09:21:58 +02:00
225df16d81 temp-sculpt-dyntopo: Fix a large number of memory leaks
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
c40d7cc278
temp-sculpt-dyntopo: Improve dyntopo remesher quality
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
78a0e81d9a
temp-sculpt-dyntopo: Fix uv reprojection bug and cleanup code
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
bd35096a79 temp-sculpt-dyntopo: Cleanup: Move code around to avoid long rebuild times
* 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.
3ab1999b41 temp-sculpt-dyntopo: Make detail flood fill dynamic, add quality slider
* 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.
buildbot/vexp-code-experimental-coordinator Build done. Details
4de7f2b06f
temp-sculpt-dyntopo: Cleanup dyntopo code
* 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
bd6069248e temp-sculpt-dyntopo Move core dyntopo loop into EdgeQueueContext
* 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.
buildbot/vexp-code-experimental-coordinator Build done. Details
774d08bfd4
temp-sculpt-dyntopo: Improve dyntopo remeshing quality
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
b1fd3ad48f temp-sculpt-dyntopo: Dyntopo cleanup and bugfixes
* 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
ac71908680 temp-sculpt-dyntopo: Smooth and topology rake fixes
* 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
Joseph Eagar added 1 commit 2023-05-30 21:18:12 +02:00
02d4bab58d temp-sculpt-dyntopo: Remove extraneous code:
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
309cf9fb82
temp-sculpt-dyntopo: fix compile error
Joseph Eagar added 1 commit 2023-05-31 00:22:00 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
dcfb042cba
temp-sculpt-dyntopo: fix msvc error
Joseph Eagar added 1 commit 2023-05-31 00:48:55 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
64963b6556
temp-sculpt-dyntopo: Fix crash in recent cleanup commits
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
buildbot/vexp-code-experimental-coordinator Build done. Details
7235d33e57
temp-sculpt-dyntopo: Topology rake fixes
* Properly handle smooth boundaries.
Joseph Eagar added 1 commit 2023-05-31 12:50:52 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
51e5b7a33b
temp-sculpt-dyntopo: Fix arm compiler error
Joseph Eagar added 3 commits 2023-05-31 14:18:37 +02:00
cfbe3b64f7 temp-sculpt-dyntopo: Remove atomics changes
* 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.
8c99399597 temp-sculpt-dyntopo: Remove unrelated drawing code 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
f498ec60f5 temp-sculpt-dyntopo: Remove more code, and cleanup dyntopo debug code
* 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
5f6125a301 temp-sculpt-dyntopo: Remove changes to customdata
* 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
dcc6302acf temp-sculpt-dyntopo: Rewrite TableGSet in c++
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
1944b90d02
temp-sculpt-dyntopo: Fix misc. issues related to UVs
* 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
buildbot/vexp-code-experimental-coordinator Build done. Details
65251574be
temp-sculpt-dyntopo: Fix compiler error
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
buildbot/vexp-code-experimental-coordinator Build done. Details
c49db52349
temp-sculpt-dyntopo: Fix compile error again
Joseph Eagar added 1 commit 2023-06-02 13:49:49 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
dbebea6343
temp-sculpt-dyntopo: More clang-related compile errors
Joseph Eagar added 1 commit 2023-06-02 13:52:23 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
0b5cb2945f
temp-sculpt-dyntopo: another compiler error
Joseph Eagar added 1 commit 2023-06-02 14:23:22 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
21aeab573d
temp-sculpt-dyntopo: Cleanup warnings
Joseph Eagar added 1 commit 2023-06-02 14:25:33 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
78d338b208
temp-sculpt-dyntopo: Remove redundant declaration
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
9cd4f0d4d3 temp-sculpt-dyntopo: Move edge collapse code to C++.
* 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
370be3d365 temp-sculpt-dyntopo: Remove pattern based triangle subdivider
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
a974c1ebf2 temp-sculpt-dyntopo: UV and attribute reprojection changes
* 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
27bf295b3c temp-sculpt-dyntopo: Use new uv smooth code for stability smooth
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
c90f603a6d temp-sculpt-dyntopo: Cleanup gpu index buffer code
Also fixed very nasty bug in customdata code.
Joseph Eagar added 1 commit 2023-06-08 23:52:23 +02:00
ee4a37f38b temp-sculpt-dyntopo: Cleanup sculpt boundary code
* Cleaned up inconsistent function naming.
* Moved most pbvh api functions into the blender::bke::pbvh namespace.
* Edge boundary flags are now cached in an attribute like vertex
  boundary flags are.
* UV boundary flags are now only calculated when requested (and
  preserved during dyntopo remeshing).
* UV snap limit is now calculated automatically, from 1/10th
  of the average uv edge length around a vertex.
Joseph Eagar added 1 commit 2023-06-09 08:41:33 +02:00
Joseph Eagar added 1 commit 2023-06-09 10:39:08 +02:00
Joseph Eagar added 1 commit 2023-06-09 13:05:07 +02:00
3a7d6d41d7 temp-sculpt-dyntopo: Fix boundery smoothing and undo bugs
* Smoothing should prioritize hard edges over smooth ones.
* Undo wasn't properly setting SCULPTFLAG_NEED_TRIANGULATE flags.
Joseph Eagar added 2 commits 2023-06-10 00:05:56 +02:00
Joseph Eagar added 1 commit 2023-06-10 00:54:14 +02:00
b88fc8d69d temp-sculpt-dyntopo: Cleanup dyntopo code some more
Move a few functions into methods of EdgeQueueContext
Joseph Eagar added 1 commit 2023-06-10 01:24:24 +02:00
Joseph Eagar added 1 commit 2023-06-10 02:47:10 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
a5bf1a92b7
temp-sculpt-dyntopo: Fix performance regressions
* Quality now controls a time limit instead of an
  edge limit.
* Only log loop customdata when necassary.
* Removed the "Relax during remeshing" option, it was added
  to test the small amount of relaxation that's stochastically
  applied to reduce the numerical instability of collapse. Sadly
  it turned out to be necassary after all.
Joseph Eagar added 1 commit 2023-06-10 02:55:06 +02:00
Joseph Eagar added 1 commit 2023-06-10 02:55:58 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
ef919c26ed
temp-sculpt-dyntopo: fix windows compile error
Joseph Eagar added 1 commit 2023-06-10 04:06:11 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
a326423612
temp-sculpt-dyntopo: Fix edge seam smoothing bug
Joseph Eagar added 1 commit 2023-06-10 08:26:29 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
284588fd4c
temp-sculpt-dyntopo: Bugfixes
* Fixed exploding layer and draw sharp brushes
* Fixed various crashes.
Joseph Eagar added 1 commit 2023-06-10 11:55:19 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
734223b5f2
temp-sculpt-dyntopo: Fix a large number of small bugs
Joseph Eagar added 1 commit 2023-06-10 12:00:02 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
aaa30bce53
temp-sculpt-dyntopo: Fix bug in last commit
Joseph Eagar added 1 commit 2023-06-11 08:38:24 +02:00
Joseph Eagar added 2 commits 2023-06-15 07:42:42 +02:00
Joseph Eagar added 1 commit 2023-06-15 10:11:00 +02:00
Joseph Eagar added 2 commits 2023-06-15 13:22:33 +02:00
Joseph Eagar added 1 commit 2023-06-15 14:40:00 +02:00
Joseph Eagar added 1 commit 2023-06-16 07:24:12 +02:00
Joseph Eagar added 2 commits 2023-06-16 11:56:35 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
909130c7e9
temp-sculpt-dyntopo: Cleanup dyntopo code a bit more
* Fixed a few issues in the detail flood fill operator.
* The entire notion of limiting "edge steps" has been removed
  from the API (since we limit by time now).
Joseph Eagar added 2 commits 2023-06-18 13:09:34 +02:00
Joseph Eagar added 1 commit 2023-06-19 14:07:51 +02:00
3a7b4cb0ff temp-sculpt-dyntopo: Split GPU normals of marked sharp edges
TODO: use autosmooth settings to split by face angles too.
Joseph Eagar added 1 commit 2023-06-19 14:11:55 +02:00
Joseph Eagar added 1 commit 2023-06-19 14:45:56 +02:00
c29549339b temp-sculpt-dyntopo: Fix edge boundary flag update bug
Fixes broken face set boundary smoothing.
Joseph Eagar added 1 commit 2023-06-21 10:47:56 +02:00
Joseph Eagar added 1 commit 2023-06-21 14:36:18 +02:00
Joseph Eagar added 1 commit 2023-06-23 08:40:42 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
e037eab4b9
temp-sculpt-dyntopo: Fix a few minor bugs
* Only run displacement erase/smear with PBVH_GRIDS
* Fix mesh filter undo bug
Joseph Eagar added 1 commit 2023-06-23 09:49:05 +02:00
Joseph Eagar added 3 commits 2023-06-23 11:50:33 +02:00
1e8603a878 temp-sculpt-dyntopo: Remove detail_range parameter
Detail_range controls how much edges have to deviate
from the goal length before remeshing kicks in.

It's a confusing paramter and 0.4f seems to work for
most cases.
Joseph Eagar added 1 commit 2023-06-23 12:33:43 +02:00
Joseph Eagar added 1 commit 2023-06-23 12:38:33 +02:00
Joseph Eagar added 1 commit 2023-06-24 10:06:25 +02:00
Joseph Eagar added 1 commit 2023-06-25 15:35:29 +02:00
Joseph Eagar added 3 commits 2023-06-27 12:22:17 +02:00
Joseph Eagar added 1 commit 2023-06-27 19:11:38 +02:00
4a134abfa1 temp-sculpt-dyntopo: Fix UV reprojection and a few other fixes
* Loop_is_corner now falls back to an angle test if the
  number of charts around a vertex is less than 2.
* Dyntopo now starts at first dab.
* Fixed crash.
Joseph Eagar added 1 commit 2023-06-27 19:58:05 +02:00
Joseph Eagar added 2 commits 2023-06-28 12:32:17 +02:00
21f7238e76 temp-sculpt-dyntopo: Remove dyntopo inherit flags from UI
* Inherit flags are now hardcoded in BKE_brush_dyntopo_inherit_flags.
* DynTopoSettings.inherit has been removed from RNA.
* There are now "overriden" properties in DynTopoSetting's RNA,
  e.g. is_subdivide_overriden.  This is used to show a warning
  checkbox when a setting is local to a brush (as in the simplify
  or snake hook brushes).
buildbot/vexp-code-experimental-coordinator Build done. Details
a0e1b02cc6
temp-sculpt-dyntopo: Rewrite uv collapse and other fixes
* Rewrote edge collapse's UV collapse code.  It now uses
  C++ and is hopefully clearer and less buggy.
* Fixed recently introduced dyntopo spacing bug.
* Fixed vertex paint not working.
Joseph Eagar added 1 commit 2023-06-28 14:02:34 +02:00
Joseph Eagar added 1 commit 2023-06-28 18:46:53 +02:00
657103d0cc temp-sculpt-dyntopo: Fix anchored and drag dot modes for dyntopo
* Anchored and drag dot now work with dyntopo
* They also now only undo the parts of the mesh they changed.
Joseph Eagar added 3 commits 2023-06-30 06:02:42 +02:00
37fb533ce5 temp-sculpt-dyntopo: Fix more performance regressions
* Use BM_log_face_if_modified instead of BM_log_face_modified.
  This ensures modified face data is only written once per
  stroke.
* BM_log_vert_before_modified now uses same logic as
  BM_log_face_if_modified
* Both functions now work by maintaining a Set inside of
  the owning BMLogEntry instead of checking each entry
  differential subset inside the entry manually.
Joseph Eagar added 1 commit 2023-06-30 10:35:38 +02:00
Joseph Eagar added 1 commit 2023-06-30 13:27:56 +02:00
Joseph Eagar added 1 commit 2023-07-01 00:22:16 +02:00
00d4fa3dec temp-sculpt-dyntopo: Fix uv collapse bug
Apparently you have to use minmax instead of centroid
when collapsing UV edges.  I thought I had all the
weights summing to produce the same result, but apparently
not.  This is also how editmode does it btw, that's where
I finally found the solution.

In other news, there is now a debug option to show UV edges
in the uv editor in sculpt mode (in dynamic topology mode).
It's purely for testing and is not meant for master.
To use, simply uncomment the DEBUG_SHOW_SCULPT_BM_UV_EDGES definition
in BKE_sculpt.h.
Joseph Eagar added 1 commit 2023-07-01 00:27:30 +02:00
4731773294 temp-sculpt-dyntopo: Yet again tweak num. stability smoothing factor
I may have to come up with some sort of hueristic for this.
Joseph Eagar added 1 commit 2023-07-01 00:31:42 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
c269861369
temp-sculpt-dyntopo: Remove debug timer
Joseph Eagar added 1 commit 2023-07-01 00:48:59 +02:00
71148abcd1 temp-sculpt-dyntopo: Don't update sharp angle flags when not requested
This was happening inside the dyntopo remesher loop,
which led to mesh artifacts.
Joseph Eagar added 1 commit 2023-07-01 00:49:44 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
24c5536e46
temp-sculpt-dyntopo: Fix compile error
Joseph Eagar added 1 commit 2023-07-01 01:00:52 +02:00
Joseph Eagar added 1 commit 2023-07-01 01:01:21 +02:00
Joseph Eagar added 3 commits 2023-07-04 10:22:51 +02:00
Joseph Eagar added 2 commits 2023-07-09 10:09:43 +02:00
ae46f6ed02 temp-sculpt-dyntopo: View sculpt bmesh attributes in spreadsheet
For debugging use only, uncomment `//#define DEBUG_SCULPT_BM_ATTRS`
in spreadsheet_data_source_geometry.cc.  Also wrote a basic
BMesh attribute GVArray backend.

To display hidden attributes, enter `bpy.app.debug_value = 892`
into the python console.
Joseph Eagar added 1 commit 2023-07-09 10:26:31 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
eb47c52706
temp-sculpt-dyntopo: Cleanup some code
Joseph Eagar added 1 commit 2023-07-09 10:37:28 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
d9c88f32a5
temp-sculpt-dyntopo: remove unused code
Joseph Eagar added 2 commits 2023-07-11 20:41:14 +02:00
Joseph Eagar added 1 commit 2023-07-12 11:17:08 +02:00
Joseph Eagar added 1 commit 2023-07-13 09:56:09 +02:00
Joseph Eagar added 1 commit 2023-07-13 10:53:38 +02:00
Joseph Eagar added 1 commit 2023-07-13 11:07:36 +02:00
e53c1fb7da temp-sculpt-dyntopo: Remove extra shapekey code in bmesh conversion
We probably do need it, but I'm not sure it survived all the
repeated merging so I'm going to do it again.
Joseph Eagar added 1 commit 2023-07-13 17:32:24 +02:00
Joseph Eagar added 1 commit 2023-07-13 18:13:07 +02:00
Joseph Eagar added 3 commits 2023-07-13 20:22:53 +02:00
Joseph Eagar added 1 commit 2023-07-17 18:52:53 +02:00
Joseph Eagar added 1 commit 2023-07-17 18:59:28 +02:00
Joseph Eagar added 1 commit 2023-07-22 00:16:12 +02:00
5e9da50bc5 temp-sculpt-dyntopo: Fix snake hook spacing bug
Brush spacing didn't work for snake hook, which
instead relied on INBETWEEN_MOUSEMOVE to throttle
mousemove events.  Since snake hook is an incremental
tool this is not correct and led to frustratingly
inconsistent behavior.

Brush spacing now works with snake hook.
Joseph Eagar added 1 commit 2023-07-22 03:23:51 +02:00

I asked Sergey to analyze how he would suggest the patch to be split.

This is intended to map out what the initial future step would be for the development team. But it can also serve as a guideline in case you, Joe, wants to keep iterating on the patch (given that you still did some updates on it recently):

Split Strategy for Dyntopo

General notes

The presentation of the project needs to be updated.

The project is not about refactoring, but about bringing new functionality: performance and attribute preservation. Refactoring by its definition does not change an external behavior.

The description needs to be updated to become a single-read self-sufficient information. Start with the description of the problem the patch solves, description how it is done.

Referring to state of other branches and their history should be avoided in the general part of the PR description. It is fine to have such information later on (after the "---" marker). It is important to give context about such referring, because reviewers are not familiar with the state and history of code outside of the PR.

Inspirational read:

Splittable parts

Based on a quick pass over the patch and personal experience of structuring projects.

NotForPR

Changes marked as NotForPR should be removed prior to asking for a review.

startup.blend

It is hard to tell what exactly is changed in the file.

A lot of versioning is to be done in the versioning_default.cc. There might be a good reason to update the file, but more information about it is needed.

MSVC + Clang changes

Similar to other platforms and compiler configuration we are open for compilation error fixes. That being said, the compilation fix needs to be submitted and presented separately.

BKE_dyntopo_set.hh

There are likely other places where iteration over unique set of elements is needed, and the implementation does not seem to be bound to Dynamic Topology. Should consider:

  • Make it a BLI primitive
  • Cover it with regression tests
  • Include (rudimentary) benchmarking, similar to the BLI_set_test.cc

BLI_heap_minmax.hh

Move to a separate PR and consider:

  • Expand the description. I.e. it states it does priority queue, so how is it comparable/different from std::priority_queue.
  • Cover with regression tests

BMesh edge collapse

The current PR description states that the current algorithm in the main branch has bugs. Having a more robust edge collapse is a general good improvement to have outside of dyntopo. It should be presented and reviewed from this pesrpective.

The implementation probably needs a re-iteration, as it is a bit strange to generate specializations of such non-trivial algorithm for all types of callbacks.

Edit BMesh fair vertices

It does not seem to be used in the patch, and the intent is not really clear. If this is a new operator which is intended to be exposed it should go into a separate PR.

CD_FLAG_TEMPORARY

Such semantic change should be isolated to own incremental step. This is likely to touch some API changes in the BKE_attribute.h and BKE_customdata.h

ATTR_NO_OPT

Either needs to be completely removed, or presented as a separate PR.

It was mentioned the change was rejected, but I did not find a PR to get more context on this topic.
From personal experience sometimes it is handy to be able to disable optimisation of a specific function during development/troubleshooting. Being able to do so without looking up for an exact syntax sounds good. So to me it does seem to be a reasonable thing to have.

BLI_mempool.h

The patch introduces BLI_mempool_get_size. Is not very clear what the difference from BLI_mempool_len is. If it does something else it needs to go to a separate review.

BMesh tool flags

The belief is that it is possible to extract this part of the patch, keeping it no-functional-changes for the current state, and review it from this perspective.

Doing so will give ability to present the need of such change and understand it and validate more efficiently, as well as removing bulk of changes related on the API change.

geometry_attributes.cc / mesh_data.cc

From reading the code it seems that it is actually a bug fix to something that is already in the main branch: make undo in sculpt mode do what one expect it to do after adding attribute.
If it is indeed so, the change needs to be split and explained what the fix mean on user level.

join_mesh_single

It seems that this is a change which Blender benefits from outside of the dynamic topology project. So seems reasonable to split the change out and review it by the modelling team.

space_image folder

Seems to be a lot of changes, and not needed for the dynamic topology project.

Auto-save changes

Needs to be split out, and seems to be already submitted as #110088
The issue is the performance and UX.

Closing notes

Surely the notes above do not cover the juicy parts of the actual dynamic topology changes. The goal so far was to bring the patch in an easier to review state. With the size of this patch it takes a lot of mental energy to even go over those easy-to-separate changes, and it will be an iterative process to tackle the entirety of the patch.

[I asked](https://devtalk.blender.org/t/2023-07-25-sculpt-texture-paint-module-meeting/30452#aligning-resources-and-developer-time-to-finish-the-dyntopo-pr-6) Sergey to analyze how he would suggest the patch to be split. This is intended to map out what the initial future step would be for the development team. But it can also serve as a guideline in case you, [Joe](https://projects.blender.org/JosephEagar), wants to keep iterating on the patch (given that you still did some updates on it recently): # Split Strategy for Dyntopo ## General notes The presentation of the project needs to be updated. The project is not about refactoring, but about bringing new functionality: performance and attribute preservation. Refactoring by its definition does not change an external behavior. The description needs to be updated to become a single-read self-sufficient information. Start with the description of the problem the patch solves, description how it is done. Referring to state of other branches and their history should be avoided in the general part of the PR description. It is fine to have such information later on (after the "---" marker). It is important to give context about such referring, because reviewers are not familiar with the state and history of code outside of the PR. Inspirational read: - https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request - https://dev.to/karaluton/a-guide-to-perfecting-pull-requests-2b66 ## Splittable parts Based on a quick pass over the patch and personal experience of structuring projects. ### NotForPR Changes marked as `NotForPR` should be removed prior to asking for a review. #### startup.blend It is hard to tell what exactly is changed in the file. A lot of versioning is to be done in the `versioning_default.cc`. There might be a good reason to update the file, but more information about it is needed. ### MSVC + Clang changes Similar to other platforms and compiler configuration we are open for compilation error fixes. That being said, the compilation fix needs to be submitted and presented separately. ### BKE_dyntopo_set.hh There are likely other places where iteration over unique set of elements is needed, and the implementation does not seem to be bound to Dynamic Topology. Should consider: * Make it a BLI primitive * Cover it with regression tests * Include (rudimentary) benchmarking, similar to the BLI_set_test.cc ### BLI_heap_minmax.hh Move to a separate PR and consider: * Expand the description. I.e. it states it does priority queue, so how is it comparable/different from std::priority_queue. * Cover with regression tests ### BMesh edge collapse The current PR description states that the current algorithm in the main branch has bugs. Having a more robust edge collapse is a general good improvement to have outside of dyntopo. It should be presented and reviewed from this pesrpective. The implementation probably needs a re-iteration, as it is a bit strange to generate specializations of such non-trivial algorithm for all types of callbacks. ### Edit BMesh fair vertices It does not seem to be used in the patch, and the intent is not really clear. If this is a new operator which is intended to be exposed it should go into a separate PR. ### CD_FLAG_TEMPORARY Such semantic change should be isolated to own incremental step. This is likely to touch some API changes in the BKE_attribute.h and BKE_customdata.h ### ATTR_NO_OPT Either needs to be completely removed, or presented as a separate PR. It was mentioned the change was rejected, but I did not find a PR to get more context on this topic. From personal experience sometimes it is handy to be able to disable optimisation of a specific function during development/troubleshooting. Being able to do so without looking up for an exact syntax sounds good. So to me it does seem to be a reasonable thing to have. ### BLI_mempool.h The patch introduces `BLI_mempool_get_size`. Is not very clear what the difference from `BLI_mempool_len` is. If it does something else it needs to go to a separate review. ### BMesh tool flags The belief is that it is possible to extract this part of the patch, keeping it no-functional-changes for the current state, and review it from this perspective. Doing so will give ability to present the need of such change and understand it and validate more efficiently, as well as removing bulk of changes related on the API change. ### geometry_attributes.cc / mesh_data.cc From reading the code it seems that it is actually a bug fix to something that is already in the main branch: make undo in sculpt mode do what one expect it to do after adding attribute. If it is indeed so, the change needs to be split and explained what the fix mean on user level. ### join_mesh_single It seems that this is a change which Blender benefits from outside of the dynamic topology project. So seems reasonable to split the change out and review it by the modelling team. ### space_image folder Seems to be a lot of changes, and not needed for the dynamic topology project. ### Auto-save changes Needs to be split out, and seems to be already submitted as #110088 The issue is the performance and UX. ## Closing notes Surely the notes above do not cover the juicy parts of the actual dynamic topology changes. The goal so far was to bring the patch in an easier to review state. With the size of this patch it takes a lot of mental energy to even go over those easy-to-separate changes, and it will be an iterative process to tackle the entirety of the patch.
Joseph Eagar added 2 commits 2023-08-15 07:42:27 +02:00
Joseph Eagar added 1 commit 2023-08-22 21:01:11 +02:00
Joseph Eagar added 1 commit 2023-08-26 09:36:30 +02:00
Joseph Eagar added 5 commits 2023-08-31 00:21:39 +02:00
86d4cb89d4 temp-sculpt-dyntopo: Don't reuse bmesh in trim operators
Preperation to revert changes to bmesh toolflags.

* The trim operators no longer reuses ss->bm (and thus don't
  depend on toolflags being a customdata layer).
* New method BKE_sculpt_set_bmesh.  It sets a new bmesh,
  frees the old one and recalculates the PBVH (if needed).
68f09fdd50 temp-sculpt-dyntopo: Remove CD_TOOLFLAGS
Revert BMesh toolflags to behaviour in master.
Joseph Eagar added 2 commits 2023-08-31 01:17:17 +02:00
Author
Member

I'm stuck maintaining this branch one way or another since I need it for my own artistic projects, so here goes.

Splittable parts

Based on a quick pass over the patch and personal experience of structuring projects.

NotForPR

Changes marked as NotForPR should be removed prior to asking for a review.

I honestly don't understand this. I explicitly created the NotForPR tag to mark code I could not remove until late in the code review process.

startup.blend

It is hard to tell what exactly is changed in the file.
A lot of versioning is to be done in the versioning_default.cc. There might be a good reason to update the file, but more information about it is needed.

Right, reverting this is a good idea and making the necessary changes in versioning_default.cc.

MSVC + Clang changes

Similar to other platforms and compiler configuration we are open for compilation error fixes. That being said, the compilation fix needs to be submitted and presented separately.

Can be reverted, yes.

BKE_dyntopo_set.hh

There are likely other places where iteration over unique set of elements is needed, and the implementation does not seem to be bound to Dynamic Topology. Should consider:

  • Make it a BLI primitive
  • Cover it with regression tests
  • Include (rudimentary) benchmarking, similar to the BLI_set_test.cc

BLI_heap_minmax.hh

Move to a separate PR and consider:

  • Expand the description. I.e. it states it does priority queue, so how is it comparable/different from std::priority_queue.
  • Cover with regression tests

It's a minmax queue, which std::priority_queue is not (i.e. it doesn't let you get the minimum as well as the maximum element).

BMesh edge collapse

The current PR description states that the current algorithm in the main branch has bugs. Having a more robust edge collapse is a general good improvement to have outside of dyntopo. It should be presented and reviewed from this pesrpective.

The implementation probably needs a re-iteration, as it is a bit strange to generate specializations of such non-trivial algorithm for all types of callbacks.

I would love to do this another way. I tried for ages to avoid having callbacks in collapse, but that required logging for undo two rings of the surrounding topology to catch all edge cases, which wasted an enormous amount of memory.

Edit BMesh fair vertices

It does not seem to be used in the patch, and the intent is not really clear. If this is a new operator which is intended to be exposed it should go into a separate PR.

I'll just revert it.

CD_FLAG_TEMPORARY

Such semantic change should be isolated to own incremental step. This is likely to touch some API changes in the BKE_attribute.h and BKE_customdata.h

That could be it's own PR, yes.

ATTR_NO_OPT

Either needs to be completely removed, or presented as a separate PR.

It was mentioned the change was rejected, but I did not find a PR to get more context on this topic.
From personal experience sometimes it is handy to be able to disable optimisation of a specific function during development/troubleshooting. Being able to do so without looking up for an exact syntax sounds good. So to me it does seem to be a reasonable thing to have.

It really is very useful for debugging.

BLI_mempool.h

The patch introduces BLI_mempool_get_size. Is not very clear what the difference from BLI_mempool_len is. If it does something else it needs to go to a separate review.

It returns the total allocated size of the memory pool,so sculpt undo can accurately report how much memory BMLog is using.

BMesh tool flags

The belief is that it is possible to extract this part of the patch, keeping it no-functional-changes for the current state, and review it from this perspective.

Reverted yesterday.

Doing so will give ability to present the need of such change and understand it and validate more efficiently, as well as removing bulk of changes related on the API change.

geometry_attributes.cc / mesh_data.cc

From reading the code it seems that it is actually a bug fix to something that is already in the main branch: make undo in sculpt mode do what one expect it to do after adding attribute.
If it is indeed so, the change needs to be split and explained what the fix mean on user level.

There is no API in master for syncing attributes with Dyntopo, because it doesn't support attributes in the first place. Thus splitting into its own patch is not possible.

join_mesh_single

It seems that this is a change which Blender benefits from outside of the dynamic topology project. So seems reasonable to split the change out and review it by the modelling team.

I might just revert it. I was planning to mark the dyntopo element ID attributes as temporary anyway, in that was they'd be no need for special logic inside join_mesh_single.

space_image folder

Seems to be a lot of changes, and not needed for the dynamic topology project.

Reverted.

Auto-save changes

Needs to be split out, and seems to be already submitted as #110088
The issue is the performance and UX.

If I could get rid of this and still attract testers I'd happily do so. Unfortunately I can't.

Closing notes

Surely the notes above do not cover the juicy parts of the actual dynamic topology changes. The goal so far was to bring the patch in an easier to review state. With the size of this patch it takes a lot of mental energy to even go over those easy-to-separate changes, and it will be an iterative process to tackle the entirety of the patch.

I'm stuck maintaining this branch one way or another since I need it for my own artistic projects, so here goes. > > ## Splittable parts > > Based on a quick pass over the patch and personal experience of structuring projects. > > ### NotForPR > > Changes marked as `NotForPR` should be removed prior to asking for a review. I honestly don't understand this. I explicitly created the NotForPR tag to mark code I could not remove until late in the code review process. > > #### startup.blend > > It is hard to tell what exactly is changed in the file. > A lot of versioning is to be done in the `versioning_default.cc`. There might be a good reason to update the file, but more information about it is needed. Right, reverting this is a good idea and making the necessary changes in `versioning_default.cc`. > > ### MSVC + Clang changes > > Similar to other platforms and compiler configuration we are open for compilation error fixes. That being said, the compilation fix needs to be submitted and presented separately. Can be reverted, yes. > > ### BKE_dyntopo_set.hh > > There are likely other places where iteration over unique set of elements is needed, and the implementation does not seem to be bound to Dynamic Topology. Should consider: > > * Make it a BLI primitive > * Cover it with regression tests > * Include (rudimentary) benchmarking, similar to the BLI_set_test.cc > > ### BLI_heap_minmax.hh > > Move to a separate PR and consider: > > * Expand the description. I.e. it states it does priority queue, so how is it comparable/different from std::priority_queue. > * Cover with regression tests > It's a minmax queue, which `std::priority_queue` is not (i.e. it doesn't let you get the minimum as well as the maximum element). > ### BMesh edge collapse > > The current PR description states that the current algorithm in the main branch has bugs. Having a more robust edge collapse is a general good improvement to have outside of dyntopo. It should be presented and reviewed from this pesrpective. > > The implementation probably needs a re-iteration, as it is a bit strange to generate specializations of such non-trivial algorithm for all types of callbacks. I would love to do this another way. I tried for ages to avoid having callbacks in collapse, but that required logging for undo two rings of the surrounding topology to catch all edge cases, which wasted an enormous amount of memory. > > ### Edit BMesh fair vertices > > It does not seem to be used in the patch, and the intent is not really clear. If this is a new operator which is intended to be exposed it should go into a separate PR. I'll just revert it. > > ### CD_FLAG_TEMPORARY > > Such semantic change should be isolated to own incremental step. This is likely to touch some API changes in the BKE_attribute.h and BKE_customdata.h > That could be it's own PR, yes. > ### ATTR_NO_OPT > > Either needs to be completely removed, or presented as a separate PR. > > It was mentioned the change was rejected, but I did not find a PR to get more context on this topic. > From personal experience sometimes it is handy to be able to disable optimisation of a specific function during development/troubleshooting. Being able to do so without looking up for an exact syntax sounds good. So to me it does seem to be a reasonable thing to have. It really is very useful for debugging. > > ### BLI_mempool.h > > The patch introduces `BLI_mempool_get_size`. Is not very clear what the difference from `BLI_mempool_len` is. If it does something else it needs to go to a separate review. It returns the total allocated size of the memory pool,so sculpt undo can accurately report how much memory BMLog is using. > > ### BMesh tool flags > > The belief is that it is possible to extract this part of the patch, keeping it no-functional-changes for the current state, and review it from this perspective. Reverted yesterday. > > Doing so will give ability to present the need of such change and understand it and validate more efficiently, as well as removing bulk of changes related on the API change. > > ### geometry_attributes.cc / mesh_data.cc > > From reading the code it seems that it is actually a bug fix to something that is already in the main branch: make undo in sculpt mode do what one expect it to do after adding attribute. > If it is indeed so, the change needs to be split and explained what the fix mean on user level. > There is no API in master for syncing attributes with Dyntopo, because it doesn't support attributes in the first place. Thus splitting into its own patch is not possible. > ### join_mesh_single > > It seems that this is a change which Blender benefits from outside of the dynamic topology project. So seems reasonable to split the change out and review it by the modelling team. I might just revert it. I was planning to mark the dyntopo element ID attributes as temporary anyway, in that was they'd be no need for special logic inside `join_mesh_single.` > > ### space_image folder > > Seems to be a lot of changes, and not needed for the dynamic topology project. > Reverted. > ### Auto-save changes > > Needs to be split out, and seems to be already submitted as #110088 > The issue is the performance and UX. > If I could get rid of this and still attract testers I'd happily do so. Unfortunately I can't. > ## Closing notes > > Surely the notes above do not cover the juicy parts of the actual dynamic topology changes. The goal so far was to bring the patch in an easier to review state. With the size of this patch it takes a lot of mental energy to even go over those easy-to-separate changes, and it will be an iterative process to tackle the entirety of the patch.
Joseph Eagar added 1 commit 2023-09-01 08:52:47 +02:00
Joseph Eagar added 2 commits 2023-09-07 23:41:21 +02:00
Joseph Eagar added 2 commits 2023-09-13 02:05:17 +02:00
Joseph Eagar added 2 commits 2023-09-22 01:48:22 +02:00
Joseph Eagar added 2 commits 2023-09-22 22:14:09 +02:00
Joseph Eagar added 2 commits 2023-09-26 22:47:04 +02:00
Joseph Eagar added 4 commits 2023-09-27 06:03:48 +02:00
Joseph Eagar added 1 commit 2023-09-27 08:59:24 +02:00
Joseph Eagar added 2 commits 2023-09-28 23:03:10 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
43d8d2b823
temp-sculpt-dyntopo: Fix remesher thrashing on link skinny faces
Also fixes bug where high-valence verts are generated during remeshing.
Joseph Eagar added 1 commit 2023-09-28 23:13:11 +02:00
Joseph Eagar added 1 commit 2023-09-28 23:42:15 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
2411cda1a7
temp-sculpt-dyntopo: Cleanup warnings
Joseph Eagar added 1 commit 2023-09-29 07:42:06 +02:00
buildbot/vexp-code-experimental-coordinator Build done. Details
cbd822132c
temp-sculpt-dyntopo: Fix crash in preview file saving
Joseph Eagar added 1 commit 2023-10-02 01:02:55 +02:00
Brecht Van Lommel removed this from the 4.0 milestone 2023-10-05 19:22:39 +02:00
Joseph Eagar added 1 commit 2023-10-07 11:13:16 +02:00
c138deefe2 temp-sculpt-dyntopo: Final fix long skinny edges
Finally figured out the proper fix for long
skinny edges, which is to fetch both the subdivide
and collapse candidate edges from the minmax
heap, then decide which leads to higher quality
by comparing how far each is from the subdivide
and collapse length limits.
Joseph Eagar added 1 commit 2023-10-07 21:03:23 +02:00
1fc7b9986b temp-sculpt-dyntopo: Fix remesher not working with dyntopo
Also fixed a couple of truly nasty undo bugs.
Joseph Eagar added 1 commit 2023-10-07 21:38:47 +02:00
Joseph Eagar added 1 commit 2023-10-07 23:14:28 +02:00
Joseph Eagar added 1 commit 2023-10-07 23:45:15 +02:00
Joseph Eagar added 1 commit 2023-10-08 23:33:40 +02:00
Joseph Eagar added 1 commit 2023-10-09 00:53:25 +02:00
Joseph Eagar added 1 commit 2023-10-09 00:56:27 +02:00
Joseph Eagar added 1 commit 2023-10-09 01:20:00 +02:00
Joseph Eagar added 1 commit 2023-10-10 01:39:07 +02:00
Joseph Eagar added 1 commit 2023-10-10 22:31:01 +02:00
Joseph Eagar added 1 commit 2023-10-10 22:40:28 +02:00
12edd719ce temp-sculpt-dyntopo: Cleanup BMIdMap some more
Use blender::Vector for BMIdMap.map
Joseph Eagar added 2 commits 2023-10-10 22:51:59 +02:00
Joseph Eagar added 1 commit 2023-10-10 22:56:36 +02:00
Joseph Eagar added 1 commit 2023-10-10 23:17:05 +02:00
Joseph Eagar added 1 commit 2023-10-10 23:23:29 +02:00
Joseph Eagar added 1 commit 2023-10-11 00:05:52 +02:00
Joseph Eagar added 2 commits 2023-10-13 21:31:32 +02:00
Joseph Eagar added 1 commit 2023-10-13 22:45:07 +02:00
Joseph Eagar added 1 commit 2023-11-23 23:03:52 +01:00
Joseph Eagar added 1 commit 2024-01-01 08:37:46 +01:00
Joseph Eagar force-pushed temp-sculpt-dyntopo from a8cbe23b08 to 368349753e 2024-01-10 08:12:00 +01:00 Compare
Joseph Eagar force-pushed temp-sculpt-dyntopo from 1cab4a42e1 to 8f9e4d6d65 2024-01-10 11:47:17 +01:00 Compare
Joseph Eagar added 1 commit 2024-02-18 20:52:11 +01:00
Joseph Eagar added 2 commits 2024-03-21 17:44:31 +01:00
This pull request has changes conflicting with the target branch.
  • source/blender/blenkernel/BKE_pbvh_api.hh
  • source/blender/blenkernel/intern/paint.cc
  • source/blender/blenkernel/intern/pbvh.cc
  • source/blender/editors/include/ED_object.hh
  • source/blender/editors/object/object_remesh.cc
  • source/blender/editors/sculpt_paint/paint_hide.cc
  • source/blender/editors/sculpt_paint/paint_vertex.cc
  • source/blender/editors/sculpt_paint/sculpt_dyntopo.cc

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin temp-sculpt-dyntopo:temp-sculpt-dyntopo
git checkout temp-sculpt-dyntopo
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104613
No description provided.