Refactor: Simplify mesh edit mode modifier evaluation #108637
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#108637
Loading…
Reference in New Issue
No description provided.
Delete Branch "HooglyBoogly/blender:cleanup-modifier-edit-mode-eval"
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?
Instead of keeping track of a local array of positions in the modifier
stack itself, use the existing edit mode SoA "edit cache" which already
contains a contiguous array of positions. Combined with positions as a
generic attribute, this means the state is contained just in the mesh
(and the geometry set) making the code much easier to follow.
To do this we make more use of the mesh wrapper system, where we can
pass a
Mesh
that's actually stored with aBMesh
and the extracached array of positions. This also resolves some confusion-- it was
weird to have the mesh wrapper system for this purpose but not use it.
Since we always created a wrapped mesh in edit mode, there's no need
for
MOD_deform_mesh_eval_get
at all anymore. That function was quiteconfusing with "eval" in its name when it really retrieved the original
mesh.
Many deform modifiers had placeholder edit mode evaluation functions.
Since these didn't do anything and since the priority is node-based
deformation now, I removed these. The case is documented more in the
modifier type struct callbacks.
It's worth landing this early in Bcon1 because there are probably some
edge cases I'm missing and this area isn't covered well by tests. But I
think the modifier stack simplification is absolutely worth it, and
opens other cleanup opportunities as well.
@blender-bot build
@blender-bot build windows
This seems reasonable to me. I think the main case we wanted to optimize with the mesh wrapper was edit mesh without modifiers, which I guess is not slower with this.
@ -1212,3 +1201,2 @@
const ModifierTypeInfo *mti = BKE_modifier_get_info((ModifierType)md->type);
if (!editbmesh_modifier_is_enabled(scene, ob, md, mesh_final != nullptr)) {
if (!editbmesh_modifier_is_enabled(scene, ob, md, i == 0)) {
I don't think
i == 0
does the same thing. This is used for multires to check that there are only deforming modifiers that don't change the topology preceding it.I guess you can just add a boolean that track if any non-deforming modifiers were applied?
@blender-bot build
Ran into some issues with this patch:
The knife tool is no longer cutting deformed geometry, I'd guess this is caused by
EditMeshData::vertexCos
being set to NULL. As tools may use this array, I think it needs to be kept. (Using the knife tool on a subdivided cube with "On Cage" in edit-mode is enough to redo the error).There is an issue where evaluation in edit-mode is slower than before when modifiers are disabled. In my tests I added a hook-modifier to a highly subdivided sphere, animated the empty object, then entered edit-mode on the mesh. Even when the modifier is disabled, animation-playback is now much slower with this patch applied. I didn't investigate this, assume the case when no recalculation needs to be performed isn't properly handled.
Noted a flipped null check inline.
@ -171,0 +169,4 @@
{
switch (mesh->runtime->wrapper_type) {
case ME_WRAPPER_TYPE_BMESH:
if (mesh->runtime->edit_data->vertexCos) {
Should be
if (mesh->runtime->edit_data->vertexCos == nullptr) {
(the hook modifier was crashing in edit-mode)Thanks for finding those issues. Both are fixed now!
The issue with the knife-tool remains (see attached screenshot).
Try add a torus with a cast modifier.
Move the torus data off-center for a more extreme difference.
With this patch applied:
Currently the "Cage" toggle isn't working in edit-mode, details in reply.
Thanks again, I misunderstood your explanation the first time. The fix to the cage option was to not ignore the wrapper type in the mesh foreach-vert function.
There is now an assert in debug mode with the hook modifier & "Edit Mode" enabled.
With this patch
MR_EXTRACT_MESH
mesh extraction used on a mesh withmr->me->runtime->wrapper_type == ME_WRAPPER_TYPE_BMESH
.The mesh fields are intentionally set to invalid values in debug mode, as they shouldn't be used (which causes the assert).
Enter edit-mode with attached file, it asserts in debug mode.
Currently there is an assertion in debug mode caused by bmesh wrapped Mesh data not using BMesh extraction.
To be clear, you're referring to only one problem, the one in your test file, right? Thanks.
@blender-bot build
Yes, the assert is in the attached blend file.
Checking this patch further, clearing
EditMeshData::vertexCos
can still cause issues.vertexNos
,polyNos
,polyCos
should be cleared too, as they are based on this data. Or, a detailed explanation for why it's necessary not to clear them as this puts theEditMeshData
in an inconsistent state.EditMeshData::vertexCos
and other members with an accessor function, since tools should be able to access the deformed (cage) coordinates & normals without complicated inline checks.I think the difference you're describing isn't really that
vertexCos
is cleared, it's that the cage mesh isn't a BMesh Mesh wrapper in some cases where it was before. It already isn't in many cases, this change just pushes that a bit further.I've added some wrapper functions for accessing the vert coords in cases where there wasn't already a separate code path for the
edit_mesh
case. In my testing it's working well now.While the update seems to be working, it's leaking memory, even in simple cases.
(wave modifier enabled in editmode on the default cube leaks
EditMeshData*
).@ -17,3 +17,3 @@
typedef struct EditMeshData {
/** when set, \a vertexNos, polyNos are lazy initialized */
const float (*vertexCos)[3];
float (*vertexCos)[3];
I think this should remain const, any changes to this wont be reflected in normals resulting in an invalid state, from what I can see manipulating this is only valid while building. Worst case assigning these values could cast to non-const with a note why it's acceptable in this particular case.
I don't think
const
is correct here-- we really do change the values in every modifier. Instead I added a separateBKE_mesh_wrapper_tag_positions_changed
function for this situation which handles clearing the cached "edit data" normals and poly centers.It's only changed in modifier evaluation, this can is also used by operators where it should be treated as const.
Unlike regular meshes, this is not actually the coordinate data, so any changes to this outside modifier evaluation put the mesh in an undefined state because changes are never written back to the mesh & any changes will be lost on any modifier evaluation.
We could even have a non-const member which is only used during modifier evaluation which is cleared once evaluation is complete. Then any calls to
BKE_mesh_wrapper_vert_coords_ensure_for_write
outside modifier evaluation can assert as this indicates incorrect API use.I think the problem is just the general lack of const correctness that comes from using pointers instead of an owning type like
Array
here. I'll change these to useArray<float3>
next if that works for you? That resolves the problem since you won't be able to retrieve a mutable pointer from aconst EditMeshData
.Not so convinced
Array<float3>
helps that much as we simply never want this changed once the modifiers are evaluated, even ifEditMeshData
is otherwise mutable.It may be impractical to use a
const
EditMeshData
as allocating the cache for normals for e.g. requires it to be modified. On the other hand this patch is otherwise fine so this change can be handled separately.Thanks, fixed the memory leak by freeing the edit data in
BKE_mesh_wrapper_ensure_mdata
. My guess is that wasn't done before because the ownership was a bit less clear.Checking again and the poly-build tool isn't showing deformed cage-vertex/edge/face locations (noticeable on subdivided cube with a cast modifier).
It's also crashing (ctrl-clicking and moving the mouse crashes withing a few seconds with ASAN enabled) which seems to be related as it's using coords from the underlying mesh instead of the edit-mesh.
@ -179,0 +188,4 @@
if (mesh->runtime->edit_data->vertexCos == nullptr) {
mesh->runtime->edit_data->vertexCos = editbmesh_vert_coords_alloc(mesh->edit_mesh);
}
return mesh->runtime->edit_data->vertexCos;
I don't think this should be possible, or -
edit_data
could store track if it's building (in debug mode), then assert if this is ever called outside of modifier evaluation.I checked in 3.6, and the poly build tool doesn't handle deformed positions correctly there either. It draws in the right place but then adds the points incorrectly. I think it makes sense to look at that separately?