Page MenuHome

Fix T77774: New undo code broken by 'make local' behavior.
ClosedPublic

Authored by Bastien Montagne (mont29) on Jun 12 2020, 5:11 PM.

Details

Summary

This is actually a nice issue due to too much optimization...

  • Making an ID local just reuse the linked one whenever possible, instead of actually making a copy of it.
  • Therefore, the collection containing that ID is seen as unchanged, since the pointer itself remained the same.
  • But on undo step, there is no way to reuse that local object, which then gets deleted, and linked one gets re-created - at a different address.
  • Collection, however, since unchanged, is not updated at all and thus keep reference to the to-be-deleted local object, instead of the linked one.
  • Issue gets even worse with viewlayers, this leads to the crash.

To address this, this patch adds a 'virtual' update flags that does nothing in update case, but will ensure that the affected IDs using the one made local are properly detected as changed across the relevant undo step.

Note that I chose to use the recalc flags mostly for a logical reason, and also because they are already properly dealt with and cleared by undo code, so this looks optimal to me.

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Jun 12 2020, 5:11 PM
Bastien Montagne (mont29) created this revision.

I'd rename ID_RECALC_VIRTUAL_UNDO to something like ID_RECALC_FOR_UNDO, to me the term "virtual" here seems quite different than typical usage in programming.

Perhaps at some point in the future we can unify handling of linked datablocks with local datablocks, using BKE_main_idmap_lookup_uuid for both to avoid the needed for this. But that would be a riskier change.

This revision is now accepted and ready to land.Jun 15 2020, 1:13 PM

Thanks for the review.

I'd rename ID_RECALC_VIRTUAL_UNDO to something like ID_RECALC_FOR_UNDO, to me the term "virtual" here seems quite different than typical usage in programming.

naming... you think too much about c++ ;) Virtual is here because it's not actually tagging for any real update, only as some kind of marker or tag for undo process.
But I don't want to fight about this, think will go for ID_RECALC_TAG_FOR_UNDO then.

Perhaps at some point in the future we can unify handling of linked datablocks with local datablocks, using BKE_main_idmap_lookup_uuid for both to avoid the needed for this. But that would be a riskier change.

That would be a nice 'cleanup friday' task I think, if that works it would greatly simplify readcode!