Added developers who have been involved in this area - if you prefer not to review this patch, feel free to remove yourselves.

The shared pointer for rigidbody and softbody is only ever copied when performing regular ID duplication (not when NO_MAIN tag is used).

I might be missing something, but I think the whole reason shared exists is to be able to share a point cache between an original and evaluated datablock. So that evaluated physics can be written into the point cache of the original datablock. And so not copying over that pointer would break that.

In D11228#286858, @Brecht Van Lommel (brecht) wrote:

The shared pointer for rigidbody and softbody is only ever copied when performing regular ID duplication (not when NO_MAIN tag is used).

I might be missing something, but I think the whole reason shared exists is to be able to share a point cache between an original and evaluated datablock. So that evaluated physics can be written into the point cache of the original datablock. And so not copying over that pointer would break that.

The sharing between copied & evaluated still works (in my tests at least) - it's just that the with this patch it's not done for all evaluated/duplicated data-blocks, only copy-on-write objects, managed by the depsgraph.

This seems to follow the depsgraph design (at least whats already there) most closely.

It might end up being simpler to flag ownership as you suggested though, In general I'd rather avoid having different ownership rules for how evaluated ID's manage shared pointers.

Ah, I see now that indeed it will still be set for the depsgraph.

Should we consider this shared pointer to be part of the object that should not be lost (just as we would not want to loose custom-properties or material list).

Not sure when you are thinking of this being lost. But generally we should preserve the point cache settings when duplicating objects or making object instances real?

This leaves the shared pointer set to NULL, while I couldn't cause a case where this caused a NULL pointer de-reference, it's possible there are situations where the data can be NULL where as before it wouldn't have been.

Functions like rna_SoftBodyModifier_point_cache_get dereference this pointer without checking. I don't know if there is a situation where it can be made to crash right now, but to me it seems like a fragile assumption.

Is there a specific reason we can't use LIB_TAG_COPIED_ON_WRITE instead of LIB_TAG_NO_MAIN everywhere for the point cache sharing?

In D11228#287003, @Brecht Van Lommel (brecht) wrote:

Ah, I see now that indeed it will still be set for the depsgraph.

Should we consider this shared pointer to be part of the object that should not be lost (just as we would not want to loose custom-properties or material list).

Not sure when you are thinking of this being lost. But generally we should preserve the point cache settings when duplicating objects or making object instances real?

Currently there aren't easy ways to trigger this (unless you count running rna_SoftBodyModifier_point_cache_get on evaluated data), if BKE_libblock_management_main_add was used anywhere this could cause a NULL shared pointer.

If we support loading data into the local Main using BLO_library_temp_load_id, that would cause issues too.

This leaves the shared pointer set to NULL, while I couldn't cause a case where this caused a NULL pointer de-reference, it's possible there are situations where the data can be NULL where as before it wouldn't have been.

Functions like rna_SoftBodyModifier_point_cache_get dereference this pointer without checking. I don't know if there is a situation where it can be made to crash right now, but to me it seems like a fragile assumption.

Right, either we would need to add NULL checks everywhere, or at least ensure it exists before accessing, in that case it would loose the point cache from the original.

Is there a specific reason we can't use LIB_TAG_COPIED_ON_WRITE instead of LIB_TAG_NO_MAIN everywhere for the point cache sharing?

We could, we'd need to either add LIB_ID_CREATE_COPIED_ON_WRITE or tag the ID LIB_TAG_COPIED_ON_WRITE before copying data to the destination.

Looking into this, the flag is never cleared at the moment, so as long as it's documented that clearing this flag changes pointer allocation logic, this could work (I'll look into it).

This is an alternative fix that ties the LIB_TAG_COPIED_ON_WRITE flag with shared pointer use: P2115

It has the advantage that depsgraph COW copies are an exception, and they're the only case where physics pointers are shared.

P2115 looks good to me.