LibOverrides - Refactor how diffing of RNA collections is handled, and extend diffing possibilities. #82160

New Issue

Bastien Montagne · 2020-10-27T17:31:02+01:00

Bastien Montagne commented

2020-10-27 17:31:02 +01:00

Status:

Milestone 1 has been implemented in Blender 3.0.

It implies forward compatibility breakage (i.e. files saved in Blender 3.0 won't open in a fully correct state regarding inserted local override modifiers/constraints, in older versions). Some fix will be added to Blender 2.93 LTS to at least prevent crashes.

Milestone 2 is not yet scheduled.

Description
Big picture:

The insertion of new items in existing RNA collections of override data-blocks (e.g. object constraints or modifiers) was 'hacked' in in early stage of the project and unfortunately never properly reworked before it got into production.

It currently has a weird implementation and design flows that make it close to impossible to extend in practice (to support e.g. re-ordering, deletions, etc.)

Goals:

Make current code match expected design of data model used to store overrides.
Support more complex operations over collection items (re-ordering, deletion?).

List Operations Design:
Ultimately this would include Insertion, possibly re-ordering of existing (from linked data) items. Supporting deletion operation remains an open topic, as it has fairly complicated implications, and is not strictly needed since items can usually be muted/de-activated anyway.

Implementation comments are in italic below.

Current Design:
Order of operations defined in the override property is crucial, since if you insert a new item A, and then another item B after A, you will need to use A as anchor for B insertion. Insertion:
Insertion operations are applied in a separate, second pass after all other override operations have been applied.* This is a mistake, insertion and ordering should be applied before any other override operation, such that order of items in destination data fully matches the one it had when operations were generated by diffing code). This is especially important for index-based operations or index-only lists.
If items in the collection can be referenced by unique names (e.g. modifiers, constraints), new items can be inserted anywhere, using any other existing item as anchor (insertion before/after that anchor item). If items in the collection can only be referenced by index (NLA tracks), new items can only be inserted after existing ones (i.e. after items coming from the linked reference datablock).
If name reference is defined, it takes precedence over index reference. If no valid reference is found, insertion point is assumed to be first (for insert after) or last (for insert before) position in the list.
*** Current implementation main flaw: it uses a single reference information, subitem_local_name/subitem_local_index. This is used to find the anchor item after which the new one (matching current override operation) is inserted, in local override data. The one to be inserted is then always considered to be the next item in the list of the local (source, stored-in-blend) override data. This makes insert after operation the only possible one currently.
*** Items inserted in local override data have a specific flag, different for each type (e.g. eModifierFlag_OverrideLibrary_Local for modifiers, CONSTRAINT_OVERRIDE_LIBRARY_LOCAL for constraints, etc.).
Re-ordering:* Not supported.
Target Design:
Insertion:* Fix current main issue by adding a third set of subitem data, called e.g. subitem_storage_name/subitem_storage_index, to be used by insertion operations to find the source item from the local override data used as storage, when re-applying overrides over linked localized data.
**** Versioning code could then simply detect that an insert operation has a define subitem_local_xxx info, but no subitem_storage_xxx one, and re-generate that one from the next item in the list of the local stored override data.
*The rest of the current design for insertions should then be fine. Re-ordering:
***This should only be supported for lists that can reference items by their names (Index-based ones would be too prone to produce invalid data if linked reference data changes).Engineer plan:
Research the best way to do_version to use existing data model in the expected way for current, already implemented code/supported feature. This might imply a breaking of forward compatibility.
Research more advanced algorithms able to detect deletion or re-ordering of existing items.

Work plan

Milestone 1 - Refactor Existing Code/Feature 33c5e7bcd5, ec71054a9b
- Change existing diffing code to generate expected override info (relatively easy).
- Versioning for old .blend files (backward compatibility, relatively easy).
- ~~Try to find a way to ensure forward compatibility (not sure this is possible without unreasonable changes to the override data structures).~~ Not practical to implement.
- Backport simple fix to 2.93 (and possibly 2.83) to avoid crashes on opening new 3.0 files. 95c82513ca 1d8d6c2f62
Milestone 2 - Advanced Diffing
- Implement a diffing algorithm that would support re-ordering and deletion of existing items.

Branch: ...

Relevant links:

...

**Status:** Milestone 1 has been implemented in Blender 3.0. It implies forward compatibility breakage (i.e. files saved in Blender 3.0 won't open in a fully correct state regarding inserted local override modifiers/constraints, in older versions). Some fix will be added to Blender 2.93 LTS to at least prevent crashes. Milestone 2 is not yet scheduled. --- **Description** **Big picture:** The insertion of new items in existing RNA collections of override data-blocks (e.g. object constraints or modifiers) was 'hacked' in in early stage of the project and unfortunately never properly reworked before it got into production. It currently has a weird implementation and design flows that make it close to impossible to extend in practice (to support e.g. re-ordering, deletions, etc.) **Goals**: * Make current code match expected design of data model used to store overrides. * Support more complex operations over collection items (re-ordering, deletion?). **List Operations Design:** Ultimately this would include `Insertion`, possibly `re-ordering` of existing (from linked data) items. Supporting `deletion` operation remains an open topic, as it has fairly complicated implications, and is not strictly needed since items can usually be muted/de-activated anyway. *Implementation comments are in italic below.* * Current Design: **Order of operations defined in the override property is crucial, since if you insert a new item `A`, and then another item `B` after `A`, you will need to use `A` as anchor for `B` insertion.** Insertion: ***Insertion operations are applied in a separate, second pass after all other override operations have been applied.**** *This is a mistake, insertion and ordering should be applied **before** any other override operation, such that order of items in destination data fully matches the one it had when operations were generated by diffing code). This is especially important for index-based operations or index-only lists.* ***If items in the collection can be referenced by unique names (e.g. modifiers, constraints), new items can be inserted anywhere, using any other existing item as anchor (insertion before/after that anchor item).*** If items in the collection can only be referenced by index (NLA tracks), new items can only be inserted after existing ones (i.e. after items coming from the linked reference datablock). ***If name reference is defined, it takes precedence over index reference.*** If no valid reference is found, insertion point is assumed to be first (for insert after) or last (for insert before) position in the list. *** ***Current implementation main flaw:** it uses a single reference information, `subitem_local_name`/`subitem_local_index`. This is used to find the anchor item after which the new one (matching current override operation) is inserted, **in local override data**. The one to be inserted is then always considered to be the next item in the list of the local (source, stored-in-blend) override data. This makes `insert after` operation the only possible one currently.* *** *Items inserted in local override data have a specific flag, different for each type (e.g. `eModifierFlag_OverrideLibrary_Local` for modifiers, `CONSTRAINT_OVERRIDE_LIBRARY_LOCAL` for constraints, etc.).* **Re-ordering:*** Not supported. * Target Design: **Insertion:*** Fix current main issue by adding a third set of subitem data, called e.g. `subitem_storage_name`/`subitem_storage_index`, to be used by insertion operations to find the source item from the local override data used as storage, when re-applying overrides over linked localized data. **** *Versioning code could then simply detect that an insert operation has a define `subitem_local_xxx` info, but no `subitem_storage_xxx` one, and re-generate that one from the next item in the list of the local stored override data.* ***The rest of the current design for insertions should then be fine.** Re-ordering: ***This should only be supported for lists that can reference items by their names (Index-based ones would be too prone to produce invalid data if linked reference data changes).**Engineer plan:** * Research the best way to `do_version` to use existing data model in the expected way for current, already implemented code/supported feature. This might imply a breaking of forward compatibility. * Research more advanced algorithms able to detect deletion or re-ordering of existing items. **Work plan** - [x] **Milestone 1 - Refactor Existing Code/Feature** 33c5e7bcd5, ec71054a9b - [x] Change existing diffing code to generate expected override info (relatively easy). - [x] Versioning for old .blend files (backward compatibility, relatively easy). - [x] ~~Try to find a way to ensure forward compatibility (not sure this is possible without unreasonable changes to the override data structures).~~ *Not practical to implement.* - [x] Backport simple fix to 2.93 (and possibly 2.83) to avoid crashes on opening new 3.0 files. 95c82513ca 1d8d6c2f62 - [ ] **Milestone 2 - Advanced Diffing** - [ ] Implement a diffing algorithm that would support re-ordering and deletion of existing items. **Branch**: ... --- **Relevant links**: * ...

Bastien Montagne self-assigned this 2020-10-27 17:31:03 +01:00

Bastien Montagne commented

2020-10-27 17:31:03 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'