Page MenuHome

Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM' - Second solution
Needs RevisionPublic

Authored by Germano Cavalcante (mano-wii) on Jul 19 2021, 11:02 PM.

Details

Summary

This is an alternative, less invasive strategy to D11599.

The original committ was reverted due to serious issues (See rBceb612a79c7c).

The solution now is to use the same operands as ID_RECALC_GEOMETRY
with ID_RECALC_GEOMETRY_DEFORM and checking which flag was used in
BKE_object_eval_uber_data.

A similar solution is seen in BKE_movieclip_eval_update,
BKE_scene_eval_sequencer_sequences and BKE_sound_evaluate.

Note:
To read the components used in id->recalc, it was necessary to change the id_recalc_tag of the GEOMETRY component.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D11969 (branched from master)
Build Status
Buildable 15940
Build 15940: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Jul 19 2021, 11:02 PM
Germano Cavalcante (mano-wii) created this revision.
Sergey Sharybin (sergey) requested changes to this revision.Jul 21 2021, 2:32 PM
Sergey Sharybin (sergey) added inline comments.
source/blender/blenkernel/intern/object_update.c
409–417

General rule: do not use DEG_get_original_object in the evaluation functions. There are some exceptions when this is needed by design, but those are rare and isolated cases.

You should be able to use object data ID's recalc from the evaluated domain.

This revision now requires changes to proceed.Jul 21 2021, 2:32 PM
  • Zero the id_recalc_tag used in the GEOMETRY component
  • Use the id_cow recalc flag

Zeroing the id_recalc_tag of the Geometry component is a risky move,
as many objects have the geometry recalculated "indirectly" (such as in
animations or in editing an object used as a curve bevel).
In these cases the recalc value of BKE_object_eval_uber_data becomes 0.

I'm not sure if this affects the "non-intermitten" state of component evaluation.
I tested two animation files and edited the bevel object of a curve.
Apparently everything is working fine. (Performance is also not affected in these cases).

Jacques Lucke (JacquesLucke) requested changes to this revision.Aug 23 2021, 12:32 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/object_update.c
409

Can we extract this block to a separate function to keep BKE_object_eval_uber_data "clean"?

Also I think all this bit manipulation logic could be made more readable. Took me a while to understand what you are actually checking for here exactly. Maybe something like:

const IDRecalcFlag combined_recalc = ob->id.recalc | ((ID *)ob->data)->recalc;
const bool is_recalc_geometry = combined_recalc | ID_RECALC_GEOMETRY;
const bool is_recalc_deform = combined_recalc | ID_RECALC_GEOMETRY_DEFORM;

if (is_recalc_deform && !is_recalc_geometry) {
   BKE_object_data_batch_cache_deform_tag(ob->data);
}
else {
   BKE_object_data_batch_cache_dirty_tag(ob->data);
}

Note, the semantic here is not exactly the same as in your patch (As can be seen be the fact that you might not always tag the batch cache. If not tagging the batch cache sometimes is ok, then this can be changed separately.)

source/blender/depsgraph/intern/node/deg_node_component.cc
329

What is the expected impact of this change? Could it happen that now the ID_RECALC_GEOMETRY tag is missing in many cases? (I don't know what this is doing exactly)

source/blender/editors/transform/transform_convert_mesh.c
2080

Is this indirectly checking whether there are any modifiers that change the topology?
If yes, then this sounds a bit weak. It can happen that depending on vertex positions a modifier only deforms a mesh or changes the topology (especially with geometry nodes where everything can happen). It feels like checking if the topology has changed can really only be done after modifiers are applied (I'm not sure, maybe modifiers set the ID_RECALC_GEOMETRY flag when necessary. In that case, this concerrn might not apply.)

Generally, a comment for why you are doing these specific checks and what assumptions you make would be good. It is not entirely clear to me why transform code would ever tag with something else than ID_RECALC_GEOMETRY_DEFORM, since it always only deforms.

This revision now requires changes to proceed.Aug 23 2021, 12:32 PM