Refactor: Simplify mesh edit mode modifier evaluation #108637

Merged
Hans Goudey merged 38 commits from HooglyBoogly/blender:cleanup-modifier-edit-mode-eval into main 2023-07-07 13:07:22 +02:00
Member

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 a BMesh and the extra
cached 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 quite
confusing 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.

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 a `BMesh` and the extra cached 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 quite confusing 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.
Hans Goudey added 1 commit 2023-06-05 23:13:02 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
973b3981b7
Refactor: Simplify mesh edit mode modifier evaluation
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 a `BMesh` and the extra
cached 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 quite
confusing 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.
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested review from Brecht Van Lommel 2023-06-05 23:13:25 +02:00
Hans Goudey requested review from Campbell Barton 2023-06-05 23:13:25 +02:00
Hans Goudey added this to the Modeling project 2023-06-05 23:13:34 +02:00
Hans Goudey added 1 commit 2023-06-06 18:55:08 +02:00
Author
Member

@blender-bot build windows

@blender-bot build windows
Brecht Van Lommel requested changes 2023-06-08 18:07:02 +02:00
Brecht Van Lommel left a comment
Owner

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.

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?

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?
HooglyBoogly marked this conversation as resolved
Hans Goudey added 2 commits 2023-06-08 19:43:55 +02:00
Brecht Van Lommel approved these changes 2023-06-08 19:48:39 +02:00
Hans Goudey added 1 commit 2023-06-13 23:51:23 +02:00
Author
Member

@blender-bot build

@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.

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.
Campbell Barton requested changes 2023-06-16 10:30:14 +02:00
@ -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)

Should be `if (mesh->runtime->edit_data->vertexCos == nullptr) {` (the hook modifier was crashing in edit-mode)
HooglyBoogly marked this conversation as resolved
Hans Goudey added 4 commits 2023-06-16 17:52:54 +02:00
Author
Member

Thanks for finding those issues. Both are fixed now!

Thanks for finding those issues. Both are fixed now!
Hans Goudey requested review from Campbell Barton 2023-06-16 17:53:40 +02:00
Hans Goudey added 1 commit 2023-06-19 03:48:43 +02:00

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:

  • The knife tool is not cutting the deformed data.
  • Disabling the modifiers "Cage" option is no longer working (evaluation behaves as if "Cage" is always enabled).
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: - The knife tool is not cutting the deformed data. - Disabling the modifiers "Cage" option is no longer working (evaluation behaves as if "Cage" is always enabled).
Campbell Barton requested changes 2023-06-20 03:20:13 +02:00
Campbell Barton left a comment
Owner

Currently the "Cage" toggle isn't working in edit-mode, details in reply.

Currently the "Cage" toggle isn't working in edit-mode, details in reply.
Hans Goudey added 10 commits 2023-06-20 17:33:25 +02:00
Author
Member

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.

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.
Hans Goudey requested review from Campbell Barton 2023-06-20 17:35:13 +02:00

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 with mr->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.

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 with `mr->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.
Campbell Barton requested changes 2023-06-21 12:16:02 +02:00
Campbell Barton left a comment
Owner

Currently there is an assertion in debug mode caused by bmesh wrapped Mesh data not using BMesh extraction.

Currently there is an assertion in debug mode caused by bmesh wrapped Mesh data not using BMesh extraction.
Hans Goudey added 3 commits 2023-06-21 22:34:11 +02:00
Hans Goudey requested review from Campbell Barton 2023-06-21 22:34:32 +02:00
Author
Member

To be clear, you're referring to only one problem, the one in your test file, right? Thanks.

To be clear, you're referring to only one problem, the one in your test file, right? Thanks.
Author
Member

@blender-bot build

@blender-bot build

To be clear, you're referring to only one problem, the one in your test file, right? Thanks.

Yes, the assert is in the attached blend file.

> To be clear, you're referring to only one problem, the one in your test file, right? Thanks. Yes, the assert is in the attached blend file.
Campbell Barton requested changes 2023-06-22 03:06:12 +02:00
Campbell Barton left a comment
Owner

Checking this patch further, clearing EditMeshData::vertexCos can still cause issues.

  • Each use of this member needs to be checked that it's not NULL when it's intended to be used for deformed vertices (edit-mode edge-length overlay no longer follows the deformed mesh for e.g.).
  • If this is cleared, 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 the EditMeshData in an inconsistent state.
  • It's probably best to have a utility functions if the combinations of checks becomes involved (if they're similar to the fix for the knife tool).
  • It might be best to replace direct access to 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.
  • A short term fix could be not to clear this member as part of this patch, and handle de-duplication & accessor funcitons separately.
Checking this patch further, clearing `EditMeshData::vertexCos` can still cause issues. - Each use of this member needs to be checked that it's not NULL when it's intended to be used for deformed vertices (edit-mode edge-length overlay no longer follows the deformed mesh for e.g.). - If this is cleared, `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 the `EditMeshData` in an inconsistent state. - It's probably best to have a utility functions if the combinations of checks becomes involved (if they're similar to the fix for the knife tool). - It might be best to replace direct access to `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. - A short term fix could be not to clear this member as part of this patch, and handle de-duplication & accessor funcitons separately.
Hans Goudey added 5 commits 2023-06-23 13:43:46 +02:00
Author
Member

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.

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.
Hans Goudey requested review from Campbell Barton 2023-06-23 13:46:27 +02:00
Hans Goudey added 1 commit 2023-06-26 14:59:50 +02:00
Campbell Barton requested changes 2023-06-30 02:57:34 +02:00
Campbell Barton left a comment
Owner

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*).

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 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.
Author
Member

I don't think const is correct here-- we really do change the values in every modifier. Instead I added a separate BKE_mesh_wrapper_tag_positions_changed function for this situation which handles clearing the cached "edit data" normals and poly centers.

I don't think `const` is correct here-- we really do change the values in every modifier. Instead I added a separate `BKE_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.

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.
Author
Member

changes are never written back to the mesh & any changes will be lost on any modifier evaluation.
If I understand right, mesh.runtime.edit_data is only ever used on evaluated meshes (which makes the name a bit wrong TBH). So this is sort of a hypothetical problem?

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 use Array<float3> next if that works for you? That resolves the problem since you won't be able to retrieve a mutable pointer from a const EditMeshData.

>changes are never written back to the mesh & any changes will be lost on any modifier evaluation. If I understand right, `mesh.runtime.edit_data` is only ever used on evaluated meshes (which makes the name a bit wrong TBH). So this is sort of a hypothetical problem? 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 use `Array<float3>` next if that works for you? That resolves the problem since you won't be able to retrieve a mutable pointer from a `const EditMeshData`.

Not so convinced Array<float3> helps that much as we simply never want this changed once the modifiers are evaluated, even if EditMeshData 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.

Not so convinced `Array<float3>` helps that much as we simply never want this changed once the modifiers are evaluated, even if `EditMeshData` 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.
Hans Goudey added 4 commits 2023-06-30 18:47:31 +02:00
Author
Member

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.

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.
Hans Goudey requested review from Campbell Barton 2023-06-30 19:09:17 +02:00
Hans Goudey added 2 commits 2023-06-30 19:09:24 +02:00
Campbell Barton requested changes 2023-07-04 05:40:02 +02:00
Campbell Barton left a comment
Owner

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.

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 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.
Hans Goudey added 3 commits 2023-07-05 19:47:34 +02:00
97aca032cf Fix poly build crash
The evaluated mesh temporarily had the old vertex array size,
we can't use the coordinates in that case. That was already checked
before but the `edit_data != null` check.
Author
Member

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?

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?
Hans Goudey requested review from Campbell Barton 2023-07-05 19:48:43 +02:00
Campbell Barton approved these changes 2023-07-07 08:57:58 +02:00
Hans Goudey merged commit 91b27ab637 into main 2023-07-07 13:07:22 +02:00
Hans Goudey deleted branch cleanup-modifier-edit-mode-eval 2023-07-07 13:07:23 +02:00
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 project
No Assignees
3 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#108637
No description provided.