LibOverride: Restore local references to virtual linked liboverrides on resync. #107144

Bastien Montagne · 2023-04-19T20:35:49+02:00

Bastien Montagne commented

2023-04-19 20:35:49 +02:00

When resyncing linked liboverride data, new IDs may be created that do
not exist in actual library file (since the lib file has not been resynced).

If such 'virtual linked liboverrides' data-blocks are used locally (e.g.
by adding such object to a local collection), on next file read they will be
detected as missing.

Now resync code will list such missing linked IDs that were
liboverrides, and try to re-use them when matching (by root name and
library) with newly generated virtual liboverrides.

The process may not be 100% perfect, especially a perfect one-to-one
remapping cannot be ensured if source data keep being changed over and
over (because of the order in which virtual linked liboverrides
generated by resync may change over time). However, in practice this
should fix the vast majority of issues, especially if sane naming
practices are used on IDs.

For the record, an attempt was made to write liboverride data together
with the placeholders for linked IDs in .blendfile. In theory, this
should ensure a perfect and fully valid re-usage of such IDs.

However, for this to work, not opnly the liboverride data of linked IDs need
to be written on disk, but also all ID references in this data has to be
considered as directly linked, to ensure that such liboverride data can
be re-read properly.

Otherwise, these placeholders would get a liboverride data with NULL ID
pointers, which is useless.

Such change feels way to intrusive for the very limited benefit, so for
now would consider current solution as the best option.

When resyncing linked liboverride data, new IDs may be created that do not exist in actual library file (since the lib file has not been resynced). If such 'virtual linked liboverrides' data-blocks are used locally (e.g. by adding such object to a local collection), on next file read they will be detected as missing. Now resync code will list such missing linked IDs that were liboverrides, and try to re-use them when matching (by root name and library) with newly generated virtual liboverrides. The process may not be 100% perfect, especially a perfect one-to-one remapping cannot be ensured if source data keep being changed over and over (because of the order in which virtual linked liboverrides generated by resync may change over time). However, in practice this should fix the vast majority of issues, especially if sane naming practices are used on IDs. --------------- For the record, an attempt was made to write liboverride data together with the placeholders for linked IDs in .blendfile. In theory, this should ensure a perfect and fully valid re-usage of such IDs. However, for this to work, not opnly the liboverride data of linked IDs need to be written on disk, but also all ID references in this data has to be considered as directly linked, to ensure that such liboverride data can be re-read properly. Otherwise, these placeholders would get a liboverride data with NULL ID pointers, which is useless. Such change feels way to intrusive for the very limited benefit, so for now would consider current solution as the best option.

🚀 1

Bastien Montagne requested review from Brecht Van Lommel 2023-04-19 20:36:22 +02:00

Bastien Montagne requested review from Julian Eisel 2023-04-19 20:36:22 +02:00

Bastien Montagne added this to the Core project 2023-04-19 20:36:29 +02:00

Bastien Montagne commented

2023-04-19 20:38:19 +02:00

Notes:

Feature requested by @eyecandy
Review expected mainly on the 'proper c++ usage' side of things here, although all input is welcome of course. ;)

@blender-bot package

Notes: * Feature requested by @eyecandy * Review expected mainly on the 'proper c++ usage' side of things here, although all input is welcome of course. ;) @blender-bot package

Blender Bot commented

2023-04-19 20:38:22 +02:00

Package build started. Download here when ready.

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

Bastien Montagne force-pushed F-liboverride-resync-replace-missing-ids from 118eb6e30f to bf5734f376

2023-04-19 21:03:17 +02:00

Compare

Bastien Montagne commented

2023-04-19 21:07:59 +02:00

@eyecandy you can find test build here: https://builder.blender.org/download/patch/PR107144/ ;)

🎉 1

Bastien Montagne commented

2023-04-19 21:09:30 +02:00

@blender-bot package

Blender Bot commented

2023-04-19 21:09:33 +02:00

Package build started. Download here when ready.

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

Bastien Montagne force-pushed F-liboverride-resync-replace-missing-ids from bf5734f376 to 87ee38fe28

2023-04-25 17:49:11 +02:00

Compare

Brecht Van Lommel requested changes 2023-04-25 18:59:24 +02:00

Brecht Van Lommel left a comment

This doesn't seem reliable, but if it helps unblock production I guess it's fine?

As I understand, if there are for example "Object.001" and "Object.002" it will not be able able to find the correct mapping. And I'd expect that to be somewhat common, not really a corner case?

This doesn't seem reliable, but if it helps unblock production I guess it's fine? As I understand, if there are for example "Object.001" and "Object.002" it will not be able able to find the correct mapping. And I'd expect that to be somewhat common, not really a corner case?

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

						
				@ -1720,1 +1722,4 @@

				/**

				 * Mapping to find suitable misisng linked liboverrides to replace by the newly generated linked

				 * liboverrides suring resync process.

Brecht Van Lommel commented

2023-04-25 18:51:10 +02:00

misisng -> missing
suring -> during?

misisng -> missing suring -> during?

Bastien Montagne commented

2023-05-06 17:01:10 +02:00

Fixed

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

						
				@ -1721,0 +1740,4 @@

				static LibOverrideMissingIDsData_Key lib_override_library_resync_missing_id_key(ID *id)

				{

				  std::string id_name_key(id->name);

				  const size_t last_key_index = id_name_key.find_last_not_of(".0123456789");

Brecht Van Lommel commented

2023-04-25 18:52:48 +02:00

This should probably also explicitly check for there to be a . as the first character of the rest of the string, so it uses test.123 but ignores test123?

This should probably also explicitly check for there to be a `.` as the first character of the rest of the string, so it uses `test.123` but ignores `test123`?

Bastien Montagne commented

2023-05-01 10:57:11 +02:00

Good point. Should also only catch .002 in test.12.002 too actually...

Good point. Should also only catch `.002` in `test.12.002` too actually...

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

						
				@ -1721,0 +1742,4 @@

				  std::string id_name_key(id->name);

				  const size_t last_key_index = id_name_key.find_last_not_of(".0123456789");

				  BLI_assert(last_key_index != std::string::npos);

Brecht Van Lommel commented

2023-04-25 18:54:30 +02:00

I don't see what guarantees there to always be a character like that in the name.

Bastien Montagne commented

2023-05-06 17:02:25 +02:00

not sure if this still needs an answer? but since we use the full ID name, it should always have at least the first two 'id code' chars, hence the assert.

Bastien Montagne force-pushed F-liboverride-resync-replace-missing-ids from 87ee38fe28 to b9d21fb386

2023-05-01 11:12:43 +02:00

Compare

Bastien Montagne force-pushed F-liboverride-resync-replace-missing-ids from b9d21fb386 to b483cfc4b4

2023-05-01 11:14:31 +02:00

Compare

Bastien Montagne commented

2023-05-01 11:26:20 +02:00

This doesn't seem reliable, but if it helps unblock production I guess it's fine?

As I understand, if there are for example "Object.001" and "Object.002" it will not be able able to find the correct mapping. And I'd expect that to be somewhat common, not really a corner case?

It actually will find the correct mapping in most cases, since the order in which this virtual linked liboverride IDs are created should remain typically the same, their 'extension' number will be stable in general. Same order is also the one in which they will be 'used' from the mapping.

Of course if things change in the source data, order may not be stable anymore, and mapping will start to be broken. But I'd consider this an acceptable limitation.

Keeping source files heavily out of sync should not be a recommended practice in the first place. And referencing virtual linked liboverrides generated from such out-of-sync state into current file is not a good idea either.

As explained in commit message above, a strong reliable behavior here is only possible if we write the override data in the linked placeholders in .blend files, and also write a reference to all of their linked reference IDs. The consequences of this change (turning a massive amount of indirectly linked IDs into directly linked ones) is imho not worth it.

> This doesn't seem reliable, but if it helps unblock production I guess it's fine? > > As I understand, if there are for example "Object.001" and "Object.002" it will not be able able to find the correct mapping. And I'd expect that to be somewhat common, not really a corner case? It actually will find the correct mapping in most cases, since the order in which this virtual linked liboverride IDs are created should remain typically the same, their 'extension' number will be stable in general. Same order is also the one in which they will be 'used' from the mapping. Of course if things change in the source data, order may not be stable anymore, and mapping will start to be broken. But I'd consider this an acceptable limitation. Keeping source files heavily out of sync should not be a recommended practice in the first place. And referencing virtual linked liboverrides generated from such out-of-sync state into current file is not a good idea either. As explained in commit message above, a strong reliable behavior here is only possible if we write the override data in the linked placeholders in .blend files, _and_ also write a reference to all of their linked reference IDs. The consequences of this change (turning a massive amount of indirectly linked IDs into directly linked ones) is imho not worth it.

Bastien Montagne force-pushed F-liboverride-resync-replace-missing-ids from b483cfc4b4 to 5e0c77594a

2023-05-01 11:36:28 +02:00

Compare

Brecht Van Lommel commented

2023-05-01 11:53:15 +02:00

Can you avoid doing force pushes when code review has started? It's difficult to see changes otherwise.

Bastien Montagne added 1 commit 2023-05-08 17:24:33 +02:00

853fcad47c Merge branch 'main' into F-liboverride-resync-replace-missing-ids

Bastien Montagne added 1 commit 2023-05-08 17:24:54 +02:00

5637384156 Fix after merging main.

Brecht Van Lommel approved these changes 2023-05-08 17:52:25 +02:00

Bastien Montagne merged commit 527b21f0ae into main