WIP: Sculpt: dynamic topology refactor #104613
Draft
Joseph Eagar
wants to merge 333 commits from
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
temp-sculpt-dyntopo
into main
pull from: temp-sculpt-dyntopo
merge into: blender:main
blender:main
blender:blender-v4.2-release
blender:npr-prototype
blender:lib_macos_png
blender:icons-cleanup
blender:blender-v3.6-release
blender:blender-v3.3-release
blender:brush-assets-project
blender:pr-extensions-tidy-space
blender:blender-v4.0-release
blender:universal-scene-description
blender:blender-v4.1-release
blender:blender-v3.6-temp_wmoss_animrig_public
blender:gpencil-next
blender:anim/animation-id-113594
blender:blender-projects-basics
blender:bridge-curves
blender:sculpt-blender
blender:asset-browser-frontend-split
blender:asset-shelf
blender:tmp-usd-python-mtl
blender:tmp-usd-3.6
blender:blender-v3.5-release
blender:blender-v2.93-release
blender:realtime-clock
blender:sculpt-dev
blender:bevelv2
blender:xr-dev
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Updated Notes
temp-sculpt-dyntopo-hive-alloc
that tends to improve performance by 10-30%. It was intended for a later PR.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:
In addition various bits and pieces of the sculpt API that were recalculated excessively or stored in undo are now stored in attributes:
.sculpt_boundary_flags
, and the API for dealing with boundaries has been improved..sculpt_valence.
.sculpt_face_areas
(as aCD_PROP_FLOAT2
, they're double-buffered for threading reasons).Other changes:
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 aCustomData
layer, instead of reallocating allBMVert/BMEdge/BMFace
structs.This is needed so sculpt operators that internally use a
BMesh
don't have to do a messyBMesh
->Mesh
->BMesh
conversion.
BMLog
is completely re-written in C++.pbvh_bmesh.cc
is split into:pbvh_bmesh.cc
: logic related to PBVH_BMESHdyntopo.cc
: main dyntopo remeshing loop, also subdivision code.dyntopo_collapse.cc
: edge collapse code.dyntopo_intern.hh
: header.EdgeQueueContext
, is in the process of being cleaned up into a more self-contained C++ class.BKE_paint.h
I added two new files,BKE_sculpt.hh
andBKE_dyntopo.hh.
I'venot really moved anything from
BKE_paint.h
itself yet though (going to do that in master), just added some new stuff..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.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
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:
Replace uses of SmallHash with blender::Map and blender::Set:
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:
Implement custom spacing for dyntopo execution.
Ref #103346
Dyntopo Refactorto #103346 Dyntopo RefactorOnly blender organization members with write access can start builds. See documentation for details.
@blender-bot build
#103346 Dyntopo Refactorto Sculpt: dynamic topology refactorSculpt: dynamic topology refactorto WIP Sculpt: dynamic topology refactorWIP Sculpt: dynamic topology refactorto WIP: Sculpt: dynamic topology refactorThis 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.
@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?
I did fix that, looks like the versioning code for it failed.
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:
2023-05-08 11-52-52.mp4
Once these are fixed, maybe it will be easier to test the branch.
Also:
Shift
to smooth, it will never use dyntopo, even if the smooth brush has "No Dyntopo" disabledOverall 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.
@JosephEagar If it helps I at least tested some the easy to find issues:
Crashes
Broken
Other
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.
@JosephEagar On the topic of the UI and some recent discussions I made some mockups.
This UI could solve multiple issues.
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.
@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.
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.
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.
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.
@ -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
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?)
@ -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?
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.
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:
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 ifprefs.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
.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.
@ -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 ."
@ -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?
@ -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.
@ -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.
@ -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?
@ -116,0 +144,4 @@
#ifdef __cplusplus
blender::Map<uintptr_t, int> vertmap;
#else
void *vertmap;
The size of
blender::Map
is not the same asvoid*
. 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++.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.
@ -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 notv.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.
@ -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.
@ -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 theatomic_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.@ -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?
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.
@ -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
andimplicit_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.
@ -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.
@ -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.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.
@ -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
.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 ofSmallHash
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.
@ -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.
@ -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.
@ -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.
@ -755,0 +778,4 @@
}
/**
* Ensure \a key is exists in \a gh.
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
?@ -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.
@ -29,6 +29,7 @@ set(INC
../makesrna
../render
../windowmanager
../../../intern/atomic
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, ¶ms);
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.
@ -35,6 +35,7 @@ struct ImBuf;
struct ImageFormatData;
struct Main;
struct MenuType;
struct PBVH;
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.
@ -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.
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.
@ -2,6 +2,7 @@
# Copyright 2006 Blender Foundation
set(INC
../../intern/atomic
Combine with other atomic header changes and move to separate PR.
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.
@blender-bot package
Package build started. Download here when ready.