Details

Reviewers

Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)

Commits

rBbfa3dc91b754: Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'

Summary

During a mesh transformation in edit mode (Move, Rotate...), not the entire draw cache needs to be recreated.

But this occurs because DEG_id_tag_update(tc->obedit->data, ID_RECALC_GEOMETRY), chain a call to BKE_object_data_batch_cache_dirty_tag that tags all the batch cache to be redone

This patch proposes not tag dirty All, but only what participates in the deformation of geometry.

Depsgraph Changes:
Currently, the graph can be compared to this simplified one:

Where DEG_id_tag_update(id, ID_RECALC_GEOMETRY) triggers geom_eval_mesh or geom_eval_obj_init.
And DEG_id_tag_update(id, ID_RECALC_SELECT) triggers update_select or update_select_obj.

This patch proposes to separate "tag_dirty" from "geom_eval" and create a separate node for it (update_all in the image bellow).
In addition to adding an node to update_deform
The graph becomes something like this:

Benchmarking:

	master:	patch:
large_mesh_editing:	Average: 16.727632 FPS	Average: 26.424897 FPS
	rdata 9ms iter 26ms (frame 60ms)	rdata 0ms iter 19ms (frame 38ms)
large_mesh_editing_ledge:	Average: 17.761902 FPS	Average: 28.070558 FPS
	rdata 9ms iter 24ms (frame 56ms)	rdata 0ms iter 18ms (frame 36ms)
looptris_test:	Average: 5.537827 FPS	Average: 5.456050 FPS
	rdata 11ms iter 26ms (frame 169ms)	rdata 11ms iter 28ms (frame 172ms)
subdiv_mesh_cage_and_final:	Average: 2.095824 FPS	Average: 2.140402 FPS
	rdata 7ms iter 21ms (frame 242ms)	rdata 0ms iter 20ms (frame 237ms)
	rdata 7ms iter 22ms (frame 233ms)	rdata 0ms iter 21ms (frame 227ms)
subdiv_mesh_final_only:	Average: 6.626541 FPS	Average: 7.974115 FPS
	rdata 3ms iter 13ms (frame 145ms)	rdata 0ms iter 10ms (frame 122ms)
subdiv_mesh_final_only_ledge:	Average: 6.590914 FPS	Average: 7.978224 FPS
	rdata 3ms iter 13ms (frame 143ms)	rdata 0ms iter 10ms (frame 121ms)

Diff Detail

Repository

rB Blender

Branch

arcpatch-D11599 (branched from master)

Build Status

Buildable 15641
Build 15641: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Jun 14 2021, 4:18 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B15169: Diff 38276.Jun 14 2021, 4:18 PM

Some might say that the graph is too complex and difficult to maintain.

Some complexity is expected for best performance.

The way ID_RECALC_GEOMETRY_DEFORM skips CoW doesn't seem to be conventional.

Yeah, unfortunately, it is not. We can add non-cow-dependent geometry component, with the operation responsible for ID_RECALC_GEOMETRY_DEFORM in there, and make current geoemtry exit operation dependent on that one. Think it could work!

source/blender/draw/intern/draw_cache_impl_mesh.c
825	Is it something to be resolved before commit?
source/blender/makesdna/DNA_ID.h
624–626	Add explanation what is it for and things like that.

Add code comments

In D11599#297337, @Sergey Sharybin (sergey) wrote:

We can add non-cow-dependent geometry component, with the operation responsible for ID_RECALC_GEOMETRY_DEFORM in there, and make current geoemtry exit operation dependent on that one.

I thought about it, I didn't implement it because I was in doubt in case it can be used for objects outside of edit mode and with CoW.
(But in this case as the mesh eval is freed together with batch cache anyway).
I'm also not sure how geometry-dependent objects behave in this case.

I will study this idea.

Harbormaster completed remote builds in B15175: Diff 38282.Jun 14 2021, 5:05 PM

source/blender/draw/intern/draw_cache_impl_mesh.c
825	I'm waiting for a decision on D11588: DRAW Cache: sanitize 'DRW_mesh_batch_cache_dirty_tag' first.

I found a small error in the graph

Isolate the operation responsible for ID_RECALC_GEOMETRY_DEFORM in its own component

Harbormaster completed remote builds in B15182: Diff 38294.Jun 14 2021, 7:13 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jun 14 2021, 7:14 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)

Germano Cavalcante (mano-wii) mentioned this in T88550: Mesh Optimization Project Progress.Jun 15 2021, 4:47 PM

Maybe it's just me, but I find it a bit confusing that there are nodes that are called "tag xxx". In the scope of the depsgraph, it's the tagging that causes the evaluation, but with these nodes it's the evaluation that causes the tagging. I know that the latter tagging is for the GPU batches and not for the depsgraph, but that's far from obvious when your brain is engaged in "understanding the depsgraph" mode. Maybe it's better to name them after what the effect should be? Like "update gpu batch" or something like that? The fact that that happens via tagging could be seen as an implementation detail.

I'm also in doubt about NodeType::GEOMETRY_NO_COW. For starters, there is no NodeType::GEOMETRY_COW. Furthermore, I'm more in favour of positive naming that express what something is for, rather than what it's not for. Wouldn't NodeType::GEOMETRY_EDIT_MODE be more appropriate?

source/blender/blenkernel/intern/object_update.c
352–382	I think this change should be separated from the rest of the patch, as it's just a non-functional change.
source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
1522	"prior actual" → "prior to actual"
source/blender/depsgraph/intern/node/deg_node_operation.h
103	Typo: `DRITY` → `DIRTY`

Sybren A. Stüvel (sybren) requested changes to this revision.Jun 18 2021, 2:52 PM

This revision now requires changes to proceed.Jun 18 2021, 2:52 PM

I think it's best to wait for a decision on the D11337: Depsgraph: remove redundant mesh data duplication in edit-mode first.
With that patch, it will no longer be necessary to create the new component (GEOMETRY_NO_COW).

Germano Cavalcante (mano-wii) mentioned this in D11337: Depsgraph: remove redundant mesh data duplication in edit-mode.Jun 18 2021, 5:20 PM

Rename GEOMETRY_NO_COW --> GEOMETRY_SHARED
Fix typos
Rename _TAG_DIRTY_ --> _UPDATE_

On second thought, D11337 doesn't directly affect this patch, so I believe that, if accepted, it can land early.
If something later changes in the behavior of the GEOMETRY component, the removal of the GEOMETRY_SHARED component may be considered.

Harbormaster completed remote builds in B15355: Diff 38595.Jun 22 2021, 2:31 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jun 22 2021, 2:31 PM

Cleanup: Remove redundant assignments of open_node

Harbormaster completed remote builds in B15356: Diff 38596.Jun 22 2021, 2:35 PM

Resolve TODO in BKE_MESH_BATCH_DIRTY_DEFORM

Harbormaster completed remote builds in B15357: Diff 38597.Jun 22 2021, 2:40 PM

Rebase on master
Remove GEOMETRY_SHARED component
Rename 'GEOMETRY_EVAL_UPDATE_DEFORM' --> 'GEOMETRY_EVAL_DEFORM'

Benchmarking:

	master:	patch:
large_mesh_editing:	Average: 16.727632 FPS	Average: 26.424897 FPS
	rdata 9ms iter 26ms (frame 60ms)	rdata 0ms iter 19ms (frame 38ms)
large_mesh_editing_ledge:	Average: 17.761902 FPS	Average: 28.070558 FPS
	rdata 9ms iter 24ms (frame 56ms)	rdata 0ms iter 18ms (frame 36ms)
looptris_test:	Average: 5.537827 FPS	Average: 5.456050 FPS
	rdata 11ms iter 26ms (frame 169ms)	rdata 11ms iter 28ms (frame 172ms)
subdiv_mesh_cage_and_final:	Average: 2.095824 FPS	Average: 2.140402 FPS
	rdata 7ms iter 21ms (frame 242ms)	rdata 0ms iter 20ms (frame 237ms)
	rdata 7ms iter 22ms (frame 233ms)	rdata 0ms iter 21ms (frame 227ms)
subdiv_mesh_final_only:	Average: 6.626541 FPS	Average: 7.974115 FPS
	rdata 3ms iter 13ms (frame 145ms)	rdata 0ms iter 10ms (frame 122ms)
subdiv_mesh_final_only_ledge:	Average: 6.590914 FPS	Average: 7.978224 FPS
	rdata 3ms iter 13ms (frame 143ms)	rdata 0ms iter 10ms (frame 121ms)

As we can see, there is a considerable improvement in the performance of files that work with mesh editing. (large_mesh_editing.blend, large_mesh_editing_ledge.blend)
But for some reason, there is a regression in the looptris_test.blend (which theoretically shouldn't be affected).
(Is it because tessellation is multitreaded and with this patch depsgraph dedicates a thread to the batch dirty tag?)
I'm not sure it's worth investigating as the penalty isn't considerable.

looptri_test.blend767 KBDownload

Harbormaster completed remote builds in B15406: Diff 38710.Jun 24 2021, 5:23 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jun 24 2021, 5:33 PM

Remove leftover from GEOMETRY_SHARED

Harbormaster completed remote builds in B15407: Diff 38712.Jun 24 2021, 5:51 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jun 24 2021, 5:53 PM

Yuichiro Fujita (chironamo) awarded a token.Jun 24 2021, 9:03 PM

Lucas (xorrito) awarded a token.Jun 24 2021, 9:57 PM

Jeroen Bakker (jbakker) awarded a token.Jun 24 2021, 10:06 PM

Adam Friesen (ace_dragon) awarded a token.Jun 24 2021, 10:35 PM

Pieter Schiettecatte (Schiette) awarded a token.Jun 25 2021, 1:38 AM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Jun 26 2021, 8:14 AM

Chris Kohl (ckohl_art) added a subscriber: Chris Kohl (ckohl_art).Jun 28 2021, 7:02 AM

Rebase on master

I checked the tests again.

Profiling: (Best and worst result)

	master:	PATCH:
large_mesh_editing:	Average: 17.137613 FPS	Average: 25.408552 FPS
	rdata 8ms iter 26ms (frame 58ms)	rdata 0ms iter 21ms (frame 39ms)
looptris_test:	Average: 5.577005 FPS	Average: 5.500189 FPS
	rdata 11ms iter 27ms (frame 168ms)	rdata 11ms iter 27ms (frame 171ms)

For experimentation, and in order to solve the worst case, I tested a new graph where the "tag_dirty" operator is called in the nodes "geom_eval_mesh_init" and "geom_eval_obj_init":

The result is better but, to work, the component "GEOMETRY" has to be added to the list of "special component where we do not want all operations to be tagged."
Although it does not seem problematic, I prefer to avoid even greater changes.

	master:	Test Graph:
large_mesh_editing:	Average: 17.137613 FPS	Average: 26.049423 FPS
	rdata 8ms iter 26ms (frame 58ms)	rdata 0ms iter 20ms (frame 38ms)
looptris_test:	Average: 5.577005 FPS	Average: 5.524559 FPS
	rdata 11ms iter 27ms (frame 168ms)	rdata 12ms iter 27ms (frame 169ms)

Harbormaster completed remote builds in B15500: Diff 38918.Jun 29 2021, 8:15 PM

silex (silex) awarded a token.Jul 1 2021, 11:38 AM

I don't quite understand the graphs. What do the "start" and "end" nodes represent? Do they mean the same thing in different graphs? I don't know much about geometry batches, so my brain has to work overtime to just follow what's being said. Having to mentally reshuffle the graphs to compare them, as each graph puts things in a different place, makes this process even harder.

source/blender/blenkernel/intern/object_update.c
400	What is this switch for? Is it here because it needs expanding for other datablock types in the future? Or will only mesh datablocks be ever supported by this function?
source/blender/depsgraph/intern/node/deg_node_operation.h
103–107	`updates` → `update`
105	`does` → `do`
105	The comment above talked about "deform batches", this comment talks about just "batches". Which "batches" are referred to here?

Add fallback for BKE_object_data_eval_batch_cache_deform_tag
Improve comments

In D11599#304298, @Sybren A. Stüvel (sybren) wrote:

What do the "start" and "end" nodes represent? Do they mean the same thing in different graphs?

The graph basically represents the Depsgraph in a simplified way. I used them to study solutions. In fact it needed more clarification for review.
The "Start" points to the nodes that start the chain of operations. For example, it points to geom_eval_mesh_deform that is equivalent to GEOMETRY_EVAL_DEFORM() in the Depsgraph. So when DEG_id_tag_update is called with the parameter ID_RECALC_GEOMETRY_DEFORM, the node equivalent to geom_eval_mesh_deform is tagged and every chain of nodes it points to is executed.
The "End" indicates that there are nodes in front but it does not need to be represented.

Harbormaster completed remote builds in B15641: Diff 39204.Jul 6 2021, 2:28 PM

Germano Cavalcante (mano-wii) added inline comments.Jul 6 2021, 3:22 PM

source/blender/blenkernel/intern/object_update.c
400	It may be supported for other datablock types in the future, but is currently only implemented for meshes. Added a fallback for the other datablocks.

Here's a little context of what currently exists (without the patch).

The cut of the graph shown below shows how the depsgraph behaves for updating the GEOMETRY component.
Note that there is no node to "update" (tag dirty) the batch cache.
This is because the BKE_object_batch_cache_dirty_tag is called along with the GEOMETRY_EVAL() operation.
(The only batch cache related node is GEOMETRY_SELECT_UPDATE()).

However, for optimization, we need to do something more granular, and not update the entire batch cache when the geometry has just been deformed.
So we need to remove the BKE_object_batch_cache_dirty_tag from GEOMETRY EVAL() and add another node that only updates part of the batch cache.
So, in the new graph, the component that already has GEOMETRY_SELECT_UPDATE() now has two new nodes: BATCH_UPDATE_DEFORM() and BATCH_UPDATE_ALL(). One node does not depend on the other and are never called together

BATCH_UPDATE_DEFORM() --> operation BKE_object_data_eval_batch_cache_deform_tag
BATCH_UPDATE_ALL() --> operation BKE_object_data_eval_batch_cache_dirty_tag

With the patch, if we call DEG_id_tag_update(mesh, ID_RECALC_GEOMETRY_DEFORM), the new node GEOMETRY_EVAL_DEFORM will be triggered and skip the BKE_object_batch_cache_dirty_tag (BATCH_UPDATE_ALL) operation.

Campbell Barton (campbellbarton) awarded a token.Jul 9 2021, 2:19 PM

Thanks for the clear explanation. +1 for more granularity.

LGTM. I didn't go through all the new depsgraph relations mentally, because I'm not that familiar with the geometry batch system anyway. I'll trust that the patch has been thoroughly tested.

This revision is now accepted and ready to land.Jul 12 2021, 5:22 PM

Closed by commit rBbfa3dc91b754: Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM' (authored by Germano Cavalcante (mano-wii)). · Explain WhyJul 12 2021, 11:06 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBbfa3dc91b754: Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'.

Fran Marz (franMarz) awarded a token.Jul 13 2021, 11:19 AM

Germano Cavalcante (mano-wii) added a reverting change: rBceb612a79c7c: Revert "Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'".Jul 19 2021, 3:18 PM

Germano Cavalcante (mano-wii) mentioned this in D11969: Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM' - Second solution.Jul 19 2021, 11:02 PM

Bataev Artem (shader) awarded a token.Jul 20 2021, 2:09 PM

Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 39204

source/blender/blenkernel/BKE_mesh_types.h

source/blender/blenkernel/BKE_object.h

source/blender/blenkernel/intern/object_update.c

source/blender/depsgraph/intern/builder/deg_builder_nodes.cc

source/blender/depsgraph/intern/builder/deg_builder_relations.cc

source/blender/depsgraph/intern/depsgraph_tag.cc

source/blender/depsgraph/intern/eval/deg_eval_flush.cc

source/blender/depsgraph/intern/node/deg_node_operation.h

source/blender/depsgraph/intern/node/deg_node_operation.cc

source/blender/draw/intern/draw_cache_impl_mesh.c

source/blender/editors/transform/transform_convert_mesh.c

source/blender/makesdna/DNA_ID.h

Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 39204

source/blender/blenkernel/BKE_mesh_types.h

source/blender/blenkernel/BKE_object.h

source/blender/blenkernel/intern/object_update.c

source/blender/depsgraph/intern/builder/deg_builder_nodes.cc

source/blender/depsgraph/intern/builder/deg_builder_relations.cc

source/blender/depsgraph/intern/depsgraph_tag.cc

source/blender/depsgraph/intern/eval/deg_eval_flush.cc

source/blender/depsgraph/intern/node/deg_node_operation.h

source/blender/depsgraph/intern/node/deg_node_operation.cc

source/blender/draw/intern/draw_cache_impl_mesh.c

source/blender/editors/transform/transform_convert_mesh.c

source/blender/makesdna/DNA_ID.h

Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'
ClosedPublic
Actions

Revision Contents
Changeset List