WIP: Sculpt: dynamic topology refactor #104613
No reviewers
Labels
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 project
No Assignees
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104613
Loading…
Reference in New Issue
No description provided.
Delete Branch "temp-sculpt-dyntopo"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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:
BLI_heap_minmax.hh
Move to a separate PR and consider:
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 fromBLI_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'm stuck maintaining this branch one way or another since I need it for my own artistic projects, so here goes.
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.
Right, reverting this is a good idea and making the necessary changes in
versioning_default.cc
.Can be reverted, yes.
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).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.
I'll just revert it.
That could be it's own PR, yes.
It really is very useful for debugging.
It returns the total allocated size of the memory pool,so sculpt undo can accurately report how much memory BMLog is using.
Reverted yesterday.
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.
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.
Reverted.
If I could get rid of this and still attract testers I'd happily do so. Unfortunately I can't.
a8cbe23b08
to368349753e
1cab4a42e1
to8f9e4d6d65
Checkout
From your project repository, check out a new branch and test the changes.