Mesh: Avoid copying positions in object mode modifier stack
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Jan 11 2023, 3:11 AM.

Details

Reviewers

Sergey Sharybin (sergey)

Maniphest Tasks

T103789: Remove unnecessary mesh position copying

Commits

rBd20f9923224d: Mesh: Avoid copying positions in object mode modifier stack

Summary

Remove the use of a separate contiguous positions array now that
they are stored that way in the first place. This allows removing the
complexity of tracking whether it is allocated and deformed in the
mesh modifier stack.

Instead of deferring the creation of the final mesh until after the
positions have been copied and deformed, create the final mesh
first and then deform its positions.

Since vertex and face normals are calculated lazily, we can rely on
individual modifiers to calculate them as necessary and simplify
the modifier stack. This was hard to change before because of the
separate array of deformed positions.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jan 11 2023, 3:11 AM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

@Sergey Sharybin (sergey) I'm adding you as a reviewer since you've reviewed one of my patches simplifying the modifier stack before.

Hans Goudey (HooglyBoogly) added a task: T103789: Remove unnecessary mesh position copying.Jan 11 2023, 3:34 AM

now that they are stored that way in the first place.

Is there a design document which explains how things are stored, when and what is lazily initialized, what and when needs to be tagged?

If there is such documentation, please make sure people are well aware of it. Like, by broadcasting it in the bf-committers in the mailing.
If there is no such documentation I would ask to put all such changes on hold, and make sure there is documentation which makes it clear what and why is happening in the code for people in an adjacent teams, and people who will be maintaining your changes later on.

How's that related to this review you might wonder. There is nothing what is obviously justifying the removal of the block which was ensuing validity of the normals before the mesh is passed to a modifier which depends on normal.
You can not rely on a reviewer digging deep into the code trying to figure out why this change is valid. It is up to the author of the patch to present the work in clear and understandable way.

This revision now requires changes to proceed.Jan 11 2023, 9:36 AM

I added some documentation here: https://wiki.blender.org/wiki/Source/Objects/Mesh
It could be expanded more, but it's also high-level documentation. I don't think "here's the functions you call for each situation" is good high-level documentation.
That's the purpose of API documentation in the headers, which already exists.

If there is such documentation, please make sure people are well aware of it. Like, by broadcasting it in the bf-committers in the mailing.

I don't really see why this development in particular needs to be broadcast on the mailing list.

It is up to the author of the patch to present the work in clear and understandable way.

Agreed, I definitely should have justified that in the patch description. It's not obvious in retrospect.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Fri, Jan 13, 8:59 PM

Iliya Katueshenock (Moder) mentioned this in T103789: Remove unnecessary mesh position copying.Mon, Jan 16, 8:42 PM

Hans Goudey (HooglyBoogly) requested review of this revision.Wed, Jan 18, 1:18 AM

It could be expanded more, but it's also high-level documentation. I don't think "here's the functions you call for each situation" is good high-level documentation.

That is what the Wiki should indeed be dedicated to: explain high level concepts. It might give some hints about areas of code and functions when it helps, but it definitely should not not be the goal to put all relevant functions and their comments to the wiki.
Think it is a good start with the page you've made. It definitely helps having a central place where things are referenced from.

Since vertex and face normals are calculated lazily

Is my understanding correct that we can remove the dependsOnNormal all together? If so, would it make sense / simply things to do it outside of this patch, so that we have a dedicated steps for "normals are lazily calculated, no need to worry about normals in the modifiers stack loop" and "avoid unnecessary position copy" ?

would it make sense / simply things to do it outside of this patch, so that we have a dedicated steps for "normals are lazily calculated, no need to worry about normals in the modifiers stack loop" and "avoid unnecessary position copy" ?

Yeah, good idea, I'll do that.

Actually, I don't think it makes sense to link removing dependsOnNormal and this change. It's still used in non-obvious ways in the edit-mode modifier stack editbmesh_calc_modifiers. It looks like eventually it will be possible to remove it, but I think there may be better cleanups to do in that area first. I'd like to improve this area in small steps, and as you noted before, this change is already not as obvious as would be ideal.