Page MenuHome

Fix depsgraphs sharing IDs via evaluated edit mesh
ClosedPublic

Authored by Sergey Sharybin (sergey) on Jan 13 2022, 10:05 AM.
Tags
None
Subscribers
None
Tokens
"Love" token, awarded by gilberto_rodrigues."Like" token, awarded by campbellbarton."Mountain of Wealth" token, awarded by jbakker.

Details

Summary

The evaluated mesh is a result of evaluated modifiers, and referencing
other evaluated IDs such as materials.
It can not be stored in the EditMesh structure which is intended to be
re-used by many areas. Such sharing was causing ownership errors causing
bugs like

T93855: Cycles crash with edit mode and simultaneous viewport and final render

The proposed solution is to store the evaluated edit mesh and its cage in
the object's runtime field. The motivation goes as following:

  • It allows to avoid ownership problems like the ones in the linked report.
  • Object level is chosen over mesh level is because the evaluated mesh is affected by modifiers, which are on the object level.

This patch allows to have modifier stack of an object which shares mesh with
an object which is in edit mode to be properly taken into account (before
the change the modifier stack from the active object will be used for all
objects which share the mesh).

There is a change in the way how copy-on-write is handled in the edit mode to
allow proper state update when changing active scene (or having two windows
with different scenes). Previously, the copt-on-write would have been ignored
by skipping tagging CoW component. Now it is ignored from within the CoW
operation callback. This allows to update edit pointers for objects which are
not from the current depsgraph and where the edit_mesh was never assigned in
the case when the depsgraph was evaluated prior the active depsgraph.

There is no user level changes changes expected with the CoW handling changes:
should not affect on neither performance, nor memory consumption.

Tested scenarios:

  • Various modifiers configurations of objects sharing mesh and be part of the same scene.

Diff Detail

Repository
rB Blender
Branch
fix_T93855_experiment (branched from master)
Build Status
Buildable 20072
Build 20072: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Jan 13 2022, 10:05 AM
Sergey Sharybin (sergey) created this revision.

First working version.

There are couple of TODOs listed in the description (will be, after I update description after updating the patch...).

From the design side the change is ready to eb reviewed.

I had some questions about ownership, some points from discussion with @Sergey Sharybin (sergey).

  • This assumes the only either the regular or edit final/cage mesh is stored since both go in data_eval. This seems to have already been the assumption before, since makeDerivedMesh will only create one or the other.
  • editmesh_eval_final and BKE_object_get_editmesh_eval_final could be removed clarify it's just pointing to the same thing as data_eval.
  • editbmesh_get_eval_cage and editbmesh_get_eval_cage_and_final set data_eval based on the edit mesh. These should only be called in edit mode, and the latter is unused even and could be removed, so this seems fine.
  • mesh_get_eval_final and mesh_get_eval_deform can set data_eval based on a regular mesh instead of an edit mesh, however here it's not obvious to me that these are safe, see inline comment.
source/blender/blenkernel/intern/DerivedMesh.cc
1968–1969

It may be that this or a similar in mesh_get_eval_deform gets called when the mesh is in editmode, potentially putting things into an invalid state.

There's many places calling these functions so it's hard to tell, probably worth seeing if we can find a case where it happens.

I think it would make sense to either call editbmesh_build_data or mesh_build_data here depending if the object is in edit mode?

For the review I did focus on the draw manager changes.

The changes in the draw manager allows to get the edit mesh from the object instance, what is fine to do.
The GPU cache is still on the mesh, changing this is IMO outside of this patch as it impacts memory requirements.

There seems to be an memory leak when editing linked duplicated meshes.

dupli_alloc BKE_editmesh_create len: 32 0x7f453f1feb68
  /home/jeroen/blender-git/build_linux/bin/blender() [0xfe90acb]
  /home/jeroen/blender-git/build_linux/bin/blender() [0xfe90bdd]
  /home/jeroen/blender-git/build_linux/bin/blender(MEM_guarded_mallocN+0x61) [0xfe90d2f]
  /home/jeroen/blender-git/build_linux/bin/blender(MEM_guarded_dupallocN+0xaa) [0xfe907b7]
  /home/jeroen/blender-git/build_linux/bin/blender(BKE_mesh_wrapper_from_editmesh_with_coords+0xbd) [0x3a43b65]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x3ec528d]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x3ec6228]
  /home/jeroen/blender-git/build_linux/bin/blender(makeDerivedMesh+0x99) [0x3ec655c]
  /home/jeroen/blender-git/build_linux/bin/blender(BKE_object_handle_data_update+0x1c5) [0x3ad026c]
  /home/jeroen/blender-git/build_linux/bin/blender(BKE_object_eval_uber_data+0x97) [0x3ad0cad]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x4497a18]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x44a34d2]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x44a088b]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x449d87e]
  /home/jeroen/blender-git/build_linux/bin/blender(_ZNKSt8functionIFvP9DepsgraphEEclES1_+0x4d) [0x4475e45]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x4474f99]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x4474fe3]
  /home/jeroen/blender-git/build_linux/bin/blender(_ZNK4TaskclEv+0x2f) [0xfe81f97]
  /home/jeroen/blender-git/build_linux/bin/blender() [0xfe82d7a]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40fd835]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40fdaeb]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40ec6e7]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40f7520]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40f955c]
  /home/jeroen/blender-git/build_linux/bin/blender() [0x40f9759]
  /lib/x86_64-linux-gnu/libc.so.6(+0x98927) [0x7f458b425927]
  /lib/x86_64-linux-gnu/libc.so.6(clone+0x44) [0x7f458b4b59e4]
Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)
  • Store evaluated mesh in the data_eval
  • Make per-object modifiers to work with different objects sharing the same mesh
  • Fixed memory leak (hopefully the same as Jeroen reported)

@Brecht Van Lommel (brecht), do you suggest to do modificiations in mesh_get_eval_final as part of this change?

@Jeroen Bakker (jbakker), think i need help with the draw manager side. I can not really figure out how to disable editmesh (verts/edges etc) overlay on non-active objects outside of edit mode. Basically, default cube, alt-d, enter edit mode. It seems that we "just" need to add some extra check somewhere where me_final is compared with me_cage (because when thsoe are not the same then draweing happens correct).

First pass on ensuring modes are not mixed (talked to Brecht, this seems to be good sanity thing to do as part of this change).

Made it so mesh_get_eval_final and mesh_get_eval_deform will not evaluated object as if it is in object mode, overriding the edit-mesh evaluated result.
The editbmesh_get_eval_cage seems to be fine, because if it is ever called with editmesh of nullptr it will crash.

The create_foo functions I tihnk best to kept unhanged. They are not expected to modify runtime object field.

In a longer term (at least, outside of this patch) think it will be proper to remove editbmesh_get_eval_cage, mesh_get_eval_deform, mesh_get_eval_final and replace with an access to already evaluated state.

@Brecht Van Lommel (brecht), @Jeroen Bakker (jbakker), Kept looking a bit into the drawing part. It kind of looks like the more proper solution would be to allow non-edit mode draw fro a mesh wrapper. Then we would be able to achieve desired behavior by returning false from DRW_object_is_in_edit_modewhen object itself is not in edit mode.

Until then something like this work P2744.
From quick thoughts it's less ideal than always using mesh wrapper, but unless i'm missing something this is not causing regression in memory consumption compared to the current master branch, as the non-active objects will create regular mesh in the scenario when two objects are sharing the same mesh data-block.

If the memory usage is indeed not harmed from the current code state I suggest move on with this patch + P2744. But would be nice to have second pair of eyes.

I guess P2744.is a performance regression for a single high poly object? As avoiding the cost of editmesh -> mesh conversion was a big reason to have the mesh wrapper in the first place, and with this patch it's always converting.

Edit: nevermind, I missed if (!(obedit->mode & OB_MODE_EDIT)) {, for the single object case it will not do any extra work.

Update with all TODOs from the personal list adressed. as well as fixes for things I could reproduce.
Think it is ready for the actual review. I'll update the description shortly.

Sergey Sharybin (sergey) retitled this revision from WIP: Fix depsgraphs sharing IDs via evaluated edit mesh to Fix depsgraphs sharing IDs via evaluated edit mesh.Jan 19 2022, 2:46 PM
Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)

Seems to work fine in my testing.

This revision is now accepted and ready to land.Jan 19 2022, 6:39 PM

Looks like BKE_modifier_get_evaluated_mesh_from_evaluated_object is always called with false in the second argument. Might be nice to remove that parameter.
With some luck the entire function simplifies to just calling BKE_object_get_evaluated_mesh then.

I wonder if we can get rid of BKE_object_get_editmesh_eval_final eventually. There doesn't really seem to be a difference between "the final evaluated mesh of the object" and "the final evaluated mesh of the object in edit mode" afaik. Using the same function to get the mesh in both cases seems reasonable to me.

I might have to follow this up with some minor tweaks to geometry-set handling because we added special cases for edit mode in a few places which might not be necessary anymore. For example, P2748 fixes an issue in the file below where the instances would disappear when the selected object goes into edit mode. That's not a critical issue as far as I can see though, so should be fine if I check that in more detail afterwards.

source/blender/blenkernel/intern/DerivedMesh.cc
1941

typos (MOTE, derivesd)

Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)

Typo fixes as pointed by Jacques.

@Jacques Lucke (JacquesLucke) for the cleanup and simplification I do agree with you. Is something we should do, but preferably as a separate patch.

The instance_edit_mode_object.blend I didn't see a regression compared to 3.0 behavior, so while its great to have the behavior fixed I think is better to apply your change afterwards (the P2748 does look fine to me btw).

Campbell Barton (campbellbarton) requested changes to this revision.Jan 24 2022, 11:39 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/object.cc
4592

This is asserting for 3d text objects, add a text object and enter-edit mode triggers the assert.

This revision now requires changes to proceed.Jan 24 2022, 11:39 AM

Good find Campbell! :)

This easiest is to ease the assert. Which I did in this iteraiton of the patch.

Cannot reproduce the memory issue anymore so that should be fixed.
I reviewed the draw manager code and am ok with the changes.

Was only thinking of adding a struct to pass the obj and mesh in a single pointer to keep the stack a bit smaller and parameters more organized. But that should not hold back this patch.

This also fixes T76609, T72733 and perhaps other reports.

This revision is now accepted and ready to land.Jan 25 2022, 9:13 AM

Ran some edit-mode benchmarks and this doesn't make a difference either way - so LGTM.