LibOverride: Add ID pointer to operations over ID pointers. #110773

Bastien Montagne · 2023-08-03T20:55:03+02:00

Bastien Montagne commented

2023-08-03 20:55:03 +02:00

In RNA collections storing ID references, the name of the collection
item may not always be unique, when several IDs from different libraries
are present.

While rare, this situation can become deadly to liboverride, by causing
random but exponential liboverride hierarchies corruptions.

This has already been alleviated by using preferably both name and index
in items lookup (a05419f18b) and by reducing the risk of name collision
in general between liboverrides and their linked reference (b9becc47de).

This commit goes further, by ensuring that references to items of RNA
collections of IDs stored in liboverride operations become completely
unambiguous. This is achieved by storing an extra pointer to the item's
ID itself, when relevant.

Lookup then requires a complete match name + ID to be successful,
which is guaranteed to match at most a single item in the whole RNA
collection (since RNA collection of IDs do not allow duplicates, and
the ID pointer is always unique).

Note that this ID pointer is implemented as an std::optional one
(either directly in C++ code, or using an new liboverride operation flag
in DNA). This allows to smoothly transition from existing data to the
added ID pointer info (when needed), without needing any dedicated
versioning. This solution also preserves forward compatibility as much
as possible.

It may also provide marginal performances improvements in some cases, as
looking up for ID items in RNA collections will first check for the
ID pointer, which should be faster than a string comparison.

Implements #110421.

NOTE: Initial patch was storing the Library pointer of the ID, however this cannot work when linking a liboverride, since Library data is always local and never linked between blendfiles. So instead this patch directly stores a 'weak reference' to the ID pointer of the RNA collection items.

In RNA collections storing ID references, the name of the collection item may not always be unique, when several IDs from different libraries are present. While rare, this situation can become deadly to liboverride, by causing random but exponential liboverride hierarchies corruptions. This has already been alleviated by using preferably both name and index in items lookup (a05419f18b) and by reducing the risk of name collision in general between liboverrides and their linked reference (b9becc47de). This commit goes further, by ensuring that references to items of RNA collections of IDs stored in liboverride operations become completely unambiguous. This is achieved by storing an extra pointer to the item's ID itself, when relevant. Lookup then requires a complete match `name + ID` to be successful, which is guaranteed to match at most a single item in the whole RNA collection (since RNA collection of IDs do not allow duplicates, and the ID pointer is always unique). Note that this ID pointer is implemented as an `std::optional` one (either directly in C++ code, or using an new liboverride operation `flag` in DNA). This allows to smoothly transition from existing data to the added ID pointer info (when needed), without needing any dedicated versioning. This solution also preserves forward compatibility as much as possible. It may also provide marginal performances improvements in some cases, as looking up for ID items in RNA collections will first check for the ID pointer, which should be faster than a string comparison. Implements #110421. -------------------------- NOTE: Initial patch was storing the Library pointer of the ID, however this cannot work when linking a liboverride, since Library data is always local and never linked between blendfiles. So instead this patch directly stores a 'weak reference' to the ID pointer of the RNA collection items.

Bastien Montagne commented

2023-08-03 20:56:40 +02:00

@blender-bot package

Blender Bot commented

2023-08-03 20:56:45 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110773) when ready.

Hans Goudey reviewed 2023-08-04 00:10:44 +02:00

source/blender/blenkernel/BKE_lib_override.hh Outdated

						
				@ -356,2 +364,4 @@

				    const char *subitem_refname,

				    const char *subitem_locname,

				    const std::optional<struct Library *> subitem_reflib,

				    const std::optional<struct Library *> subitem_loclib,

Hans Goudey commented

2023-08-04 00:10:02 +02:00

struct is unnecessary even for forward declared types in C++

`struct` is unnecessary even for forward declared types in C++

👍 1

mont29 marked this conversation as resolved

source/blender/blenkernel/intern/lib_override.cc Outdated

						
				@ -3755,0 +3767,4 @@

				  IDOverrideLibraryPropertyOperation *opop;

				  for (opop = static_cast<IDOverrideLibraryPropertyOperation *>(BLI_findstring_ptr(

				           liboverride_operations, subitem_main_name, int(offesetof_opop_main_name)));

				       opop;

Hans Goudey commented

2023-08-04 00:10:22 +02:00

Looks like this line does nothing

Bastien Montagne commented

2023-08-04 10:50:09 +02:00

It's the condition check of the for loop... We do not want to keep looping and processing over null opop pointer.

It's the condition check of the `for` loop... We do not want to keep looping and processing over null `opop` pointer.

Hans Goudey commented

2023-08-04 13:53:12 +02:00

Oh, of course, sorry-- didn't realize the for loop was spread over so many lines!

mont29 marked this conversation as resolved

Bastien Montagne force-pushed liboverride-unique-id-items-library from b7290c4da5 to 1b0c7d73ff

2023-08-04 11:39:54 +02:00

Compare

Bastien Montagne force-pushed liboverride-unique-id-items-library from 1b0c7d73ff to 503116471b

2023-08-07 12:12:27 +02:00

Compare

Bastien Montagne force-pushed liboverride-unique-id-items-library from 503116471b to 073a352554

2023-08-07 14:29:24 +02:00

Compare

Bastien Montagne changed title from ~~WIP: LibOverride: Add library pointer to operations over ID pointers.~~ to LibOverride: Add ID pointer to operations over ID pointers.

2023-08-07 14:31:50 +02:00

Bastien Montagne commented

2023-08-07 14:32:04 +02:00

@blender-bot build

Bastien Montagne force-pushed liboverride-unique-id-items-library from 073a352554 to f00c427955

2023-08-07 14:39:38 +02:00

Compare

Bastien Montagne referenced this pull request

2023-08-07 14:53:02 +02:00

Ensure name collisions between liboverrides and their reference data does not breaks liboverrides. #110421

Bastien Montagne commented

2023-08-07 14:55:01 +02:00

@brecht not sure if you want to check on this one? It implements the idea we were discussing about regarding making liboverride reference to RNA collection IDs items fully non-ambiguous.