Page MenuHome

Bypass EditMesh to Mesh conversion (initial step)
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Mar 18 2020, 4:31 AM.
Tokens
"Love" token, awarded by demmordor."Love" token, awarded by Blendork."Love" token, awarded by daviddomingues_."Like" token, awarded by candre."Love" token, awarded by lcs_cavalheiro."Love" token, awarded by 295032."Love" token, awarded by pablovazquez."Love" token, awarded by madminstrel."Love" token, awarded by weasel."Like" token, awarded by lucky3."Love" token, awarded by samgreen."Love" token, awarded by szap."Burninate" token, awarded by amonpaike."Yellow Medal" token, awarded by franMarz."Burninate" token, awarded by mywa880."Mountain of Wealth" token, awarded by EitanSomething.

Details

Summary

This is an initial patch to avoid converting EditMesh to Mesh.

This patch makes the following changes:

  • Mesh data blocks in the modifier stack can be "thin wrapped" edit-mesh data (empty mesh data, with pointers to the EditMesh and EditMeshData).
  • Any users of these meshes that need the full mesh data can ensure this data exists (delaying edit-mesh conversion until it's needed).
  • Currently, this means there is no EditMesh to Mesh conversion for edit-mode for many deform modifiers, giving significant performance improvements (about 2x).
  • RNA Object.to_mesh initialized mesh data for Cycles & exporters.

In this patch a define is checked for, currently no functional changes unless the developer defines USE_EDITMESH_THIN_WRAP (exact details of how this is enabled can be tweaked).

Once this patch is applied, each modifier with a deformVertsEM callback which runs BKE_mesh_from_editmesh_thick_wrap_ensure needs to be updated to remove this call.
In most cases this can be removed once there is a utility to get vertex weights from both a mesh and edit-mesh vertices.

Performance

Testing playback of a mesh with ~98k tris and two deform modifiers gives a little over 2x performance improvement here.

Note that draw_cache_extract_mesh.c is accessing normals and coordinates more than I would like. In the current patch this checks for deformed coordinates on each access.

Eventually we should be able to avoid passing vertex coordinates to the GPU so many times to reduce the overhead.

Currently it's not practical to split the code into separate branches for deform/non-deform cases. With different logic for updating the coordinate buffer, we might be able to avoid checking for deformed coords on every access.

Possible Changes

Here are some changes I'm considering.

  • Instead of having thin-wrapped mesh which uses a boolean to check we could have a data-type, so mesh->runtime.data_type could be: ME_GEOMTYPE_MDATA, ME_GEOMTYPE_BMESH and later perhaps ME_GEOMTYPE_SUBSURF.
  • Currently the mesh extraction code is calculating data from the edit-mesh (such as tangents and normals). This is awkward as there is logic in the modifier calculation code to calculate this for mesh data, see: editbmesh_calc_modifier_final_normals. This area could be cleaned up, making sure calculations happen in editbmesh_calc_modifier_final_normals, even with an edit-mesh.

Remaining Work

  • Ensure lazy initialization doesn't cause threading bugs. As long as the lazy initializing the mesh data happens in modifiers (for the mesh being created by the modifier stack). This isn't needed, however we will most likely want locking for the final id_eval, as well as modifiers that depend on other objects geometry.
  • Checking UV tangents are being correctly calculated before & after this patch is applied.
  • Check on custom normal editing.
  • Remove or update BKE_mesh_foreach_mapped_loop, as it's not used. This currently uses Mesh loop layer even for edit-mesh data when MESH_FOREACH_USE_NORMAL is enabled.
  • Support statvis "Distort" display option with coordinates deformed by modifiers.
  • At the moment, requesting the render mesh will always convert the edit-mesh to a regular mesh, this is caused by render-mesh always requesting ORCO's, adding an ORCO layer to an edit-mesh doesn't make sense (the original coordinates can be accessed directly). We might want to look into having a way to bypass this, since it slows down modeling with a render display mode enabled.

Proposal

  • Finish this patch to a basic level:
    • Resolve lazy initializing multi-threading for modifiers that depend on geometry.
    • Ensure tangents work correctly (needs testing).
    • Ensure normal editing works correctly (needs testing).
  • Incrementally remove BKE_mesh_from_editmesh_thick_wrap_ensure for modifiers deformVertsEM callbacks.
  • Ensure there are no regressions (UV-mapping, UV-tangents, auto-smooth, normal-editing, crazy-space, snapping to edit-mesh data).
  • Remove pre-processor checks and enable this by default.

Note: this could go into master and be disabled at any point, initially this was my plan but as the patch gets a bit noisy. I'm not as convinced this is worth doing.

Further Work

  • Support modifiers passing thin-wrapped meshes between modifiers (eModifierTypeFlag_AcceptsBMesh). This shouldn't be such a big task, it's mostly a matter of making sure there are no regressions.
  • Support bypassing normal calculation in wire-frame mode (for 2.7x performance in wire-frame mode - if we want to aim for this)

Update (April 6, 2020)

Summary of changes made since the patch was submitted:

  • Use BKE_mesh_geomtype_ for meshes which have data of another type (could rename to BKE_mesh_derived_* - suggestions welcome).
  • Added finalization check so the deferred normal calculations that happen on the final mesh are still executed when converting the final-edit-mesh to a regular mesh.
  • Tested moving edit-mesh loop-normal calculations from mesh extraction into modifier evaluation P1328, however I'd rather look into this as a separate patch to this, since this change could be applied for non-editmesh data too, and any mis-match between the mesh generated by the modifier stack and mesh extraction will cause new bugs.
  • Fixed split-normal calculation which was not using the deformed coordinates.

Diff Detail

Repository
rB Blender
Branch
TEMP-MESH-OPTIMIZE-v2 (branched from master)
Build Status
Buildable 7470
Build 7470: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Ensure mesh data is available for Object.to_mesh(), Cycles now works.

  • Fix drawing tangents in edit-mode

This seems broadly the right direction to me. My main concern is naming and conventions in API functions, how to know which ones work on a regular mesh, on a bmesh mesh, or both.

Instead of having thin-wrapped mesh which uses a boolean to check we could have a data-type, so mesh->runtime.data_type could be: ME_GEOMTYPE_MDATA, ME_GEOMTYPE_BMESH and later perhaps ME_GEOMTYPE_SUBSURF.

I think that would be better, I find the thin/thick wrap terminology unclear.

Currently the mesh extraction code is calculating data from the edit-mesh (such as tangents and normals). This is awkward as there is logic in the modifier calculation code to calculate this for mesh data, see: editbmesh_calc_modifier_final_normals. This area could be cleaned up, making sure calculations happen in editbmesh_calc_modifier_final_normals, even with an edit-mesh.

Yes, computation like that should happen in depsgraph evaluation rather than drawing code whenever possible.

source/blender/blenkernel/BKE_editmesh_cache.h
41

This can be an inline function if it's performance critical, no need for it to be a macro.

source/blender/blenkernel/intern/editmesh.c
266–267

To me it seems like BKE_mesh_minmax should have this logic, rather than every place that calls BKE_mesh_minmax?

In general we need good (naming) conventions on this in the BKE mesh API, it's quite fuzzy to me now.

General approach looks fine to me too, not much more to say at this point, did not actually check the code in details.

Instead of having thin-wrapped mesh which uses a boolean to check we could have a data-type, so mesh->runtime.data_type could be: ME_GEOMTYPE_MDATA, ME_GEOMTYPE_BMESH and later perhaps ME_GEOMTYPE_SUBSURF.

I think that would be better, I find the thin/thick wrap terminology unclear.

Fully agree as well, looks much cleaner and easier to extend to me.

  • Replace thin/thick wrapping with a mesh geometry type enum.
  • Accessor function for loop data layers.
  • Remove define define to enable/disable the new code-paths.
  • Fix accessing wrong edit mesh data from the draw manager
  • Fix crash running add_orco_mesh
  • Ensure finalization step normally run by modifier calculation, runs when converting the bmesh to a mesh, when flagged to do so.
  • Moved cd_mask_extra to Mesh_Runtime as other mesh types (subsurf) will need this for finalizing the mesh conversion.

Edit: moved to main task.

  • Use edit-mesh deformed coords for calculating split normals
  • Use un-deformed orco coordinates from the edit-mesh
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Move boundbox access back into mesh.c
  • Calculate BMesh normals or ensure edit-mesh cache normals when dependsOnNormals callback requests it
  • Add description in header
Campbell Barton (campbellbarton) marked an inline comment as not done.May 5 2020, 3:10 AM

@Bastien Montagne (mont29), @Brecht Van Lommel (brecht), @Sergey Sharybin (sergey) - anything holding this patch back, I'd like to continue development in master.

source/blender/blenkernel/intern/editmesh.c
266–267

I'd rather hold off this change for now, until it's clearer where it makes sense to abstract over types, we may want to limit this to a more isolated API as we had for derived-mesh.

If we need to abstract over mesh we can explicitly use the BKE_mesh_wrapper_* API.

If we move this abstraction into BKE_mesh_* API, it's hard to know what the author intended, since there are cases we use the original mesh even when in sculpt/edit a mode for e.g.

It also means we may get confusing bugs if some operations don't abstract over the different kinds of internal data.

If we want to make all mesh operations wrap different kinds of data, we can fairly easily do this, the reverse isn't true.

Overall this seems fine with me.

source/blender/bmesh/intern/bmesh_mesh_conv.c
1001

Remove debug print.

This revision is now accepted and ready to land.May 15 2020, 2:28 AM

Rebase against master, remove debug printf