Page MenuHome

Fix T99460: Allow creation new datablocks from evaluated
ClosedPublic

Authored by Sergey Sharybin (sergey) on Aug 3 2022, 12:12 PM.

Details

Summary

This changes makes it possible to copy evaluated result and put it
to the original bmain.

Prior to this change from the API point of view there was false
perception that it is possible, while in practice it was very fragile:
it only worked if the ID did not reference any evaluated IDs.

This change makes it so id.copy() Python API call will make it so
the copied ID only references original data-blocks. This sounds a bit
implicit, so here is motivational aspect why it is considered better
approach to all other:

  • There needs to be a way to support the described scenario, in the lest fragile way. Requiring to always use an explicit function call or an argument is too verbose and is easy to be missed.
  • The id.copy() is already doing implicit thing: it always adds the result to the bmain. So it might as well ensure the copied result does not reference evaluated data-blocks.
  • Added clarity in the documentation should address possible confusion.

The limitation of this change is that the copy() of evaluated geometry will clear its reference to the shape key. This is because the key is only referenced for validness of RNA paths for drivers and the key itself might not match topology of evaluated geometry due to modifiers.

Diff Detail

Repository
rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Aug 3 2022, 12:12 PM
Sergey Sharybin (sergey) created this revision.
Sergey Sharybin (sergey) planned changes to this revision.Aug 3 2022, 1:04 PM

Need to take care of shapekeys, as those can not be trusted from the evaluated object.

  • Properly increase user counter
  • Discard shape keys when copyign evaluated geometry
Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)Aug 3 2022, 3:43 PM
Brecht Van Lommel (brecht) requested changes to this revision.Aug 3 2022, 7:23 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/lib_id.c
750

but it -> but

source/blender/makesrna/intern/rna_ID.c
2049

.c -> .

2050

I don't think it's obvious what "original counterparts" means in this context, that this is about the depsgraph.

This revision now requires changes to proceed.Aug 3 2022, 7:23 PM

Could the added BKE_id function replace BKE_object_material_from_eval_data and maybe even BKE_mesh_nomain_to_mesh?

Re-phrase description of id.copy().

source/blender/makesrna/intern/rna_ID.c
2050

Pulling "the depsgraph" into this equation to me is even more confusing. There is no "the depsgraph". One shouldn't really care if the data-block which is being copied is coming from global bmain, evaluated state of a dependency graph, or from object.to_mesh().

I've tried to rephrase the description. If you still find it confusing it would be very useful to have some suggestions of how exactly to improve the wording.

@ HooglyBoogly, I do not see how it could be a direct replacement:

  • BKE_object_material_from_eval_data does not do anything with the data-block itself, only modifies material slots in the original object.
  • BKE_mesh_nomain_to_mesh is kinda implied in the name: it copies something outside of the bmain. The BKE_id_copy_for_use_in_bmain copies into bmain and ensures references don't go outside of the bmain.
This revision is now accepted and ready to land.Aug 4 2022, 1:07 PM

I think the new description is fine.

Bastien Montagne (mont29) requested changes to this revision.Aug 4 2022, 2:34 PM

Generally looks fine, but there are some key issues I think with current proposed code, noted in comments below.

source/blender/blenkernel/BKE_lib_id.h
457

appearing

source/blender/blenkernel/intern/lib_id.c
722

ID,

722

that the

722

with a

723

which implies that the user count of the original ID is increased

727–729

This should ONLY happen when (cb_data->cb_flag & IDWALK_CB_USER) != 0. We have a lot of ID usages that are not refcounting.

748

Are you sure about that? It means that you are not handling pointers within embedded IDs, which will therefore still potentially point to evaluated/non-Main IDs?

750

'evaluated ID', or 'evaluated object data', not 'evaluated object', object itself has no shapekey, and calling BKE_key_from_id_p on an Object ID will always return NULL...

This revision now requires changes to proceed.Aug 4 2022, 2:34 PM
source/blender/blenkernel/intern/lib_id.c
748

No, I'm not, I've misread the comment. Good catch!

Address feedback from Bastien.

This revision is now accepted and ready to land.Aug 4 2022, 3:42 PM