Fix #129207: GPv3: Linked GPv2 objects disappear when reopening file. #129220

Bastien Montagne · 2024-10-18T16:49:40+02:00

Bastien Montagne commented

2024-10-18 16:49:40 +02:00

This happens because blender 4.3 only knows about GPv3 IDs, and
therefore stores a reference to a directly linked GPv2 obdata as a GPv3
one, which cannot be found again on next file load in the GPv2 library.

The idea of this fix is, if the lookup of an idcode+idname fails, to also
search for the same name but with an older, deprecated ID code.

This happens because blender 4.3 only knows about GPv3 IDs, and therefore stores a reference to a directly linked GPv2 obdata as a GPv3 one, which cannot be found again on next file load in the GPv2 library. The idea of this fix is, if the lookup of an idcode+idname fails, to also search for the same name but with an older, deprecated ID code.

Bastien Montagne added 1 commit 2024-10-18 16:49:52 +02:00

Fix #129207 : GPv3: Linked GPv2 objects disappear when reopening file.

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

a9605b841f

This happens because blender 4.3 only knows about GPv3 IDs, and
therefore stores a reference to a directly linked GPv2 obdata as a GPv3
one, which cannot be found again on next file load in the GPv2 library.

The idea of this fix is, if the looup of a idcode+idname fails, to also
search for the same name but with an older, deprecated ID code.

Bastien Montagne commented

2024-10-18 16:50:07 +02:00

@blender-bot build

Bastien Montagne requested review from Sergey Sharybin 2024-10-18 16:50:22 +02:00

Bastien Montagne requested review from Falk David 2024-10-18 16:50:22 +02:00

Bastien Montagne commented

2024-10-18 16:54:44 +02:00

@Sergey pulling you in here as this affects quite low-level aspect of our readfile code.

TBH I do not know how I feel about this solution. On one hand it's fairly simple and I do not really see how it could turn bad/cause issues. On the other hand, it feels fuzzy and hard to foresee all potential consequences.

We could also have a more restricted and controlled way to do this fallback-lookup, but that would be more involved to find all places that need it, and would not solve the root 'weirdness' of loading a different ID type than the one required.

Or we could also just decide that this is a corner case and accept the current behavior... People should link Objects or Collections anyway, usually not obdata. ;)

@Sergey pulling you in here as this affects quite low-level aspect of our readfile code. TBH I do not know how I feel about this solution. On one hand it's fairly simple and I do not really see how it could turn bad/cause issues. On the other hand, it feels fuzzy and hard to foresee all potential consequences. We could also have a more restricted and controlled way to do this fallback-lookup, but that would be more involved to find all places that need it, and would not solve the root 'weirdness' of loading a different ID type than the one required. Or we could also just decide that this is a corner case and accept the current behavior... People should link Objects or Collections anyway, usually _not_ obdata. ;)

Sergey Sharybin reviewed 2024-10-18 17:12:06 +02:00

Sergey Sharybin left a comment

I can not think of any ceses where it could fail. It is also probably something we'd need in the future when we'd be removing legacy curves?

Alternative would be to forbid linking legacy IDs to avoid situations when non-informed artist might loose their work.

I can not think of any ceses where it could fail. It is also probably something we'd need in the future when we'd be removing legacy curves? Alternative would be to forbid linking legacy IDs to avoid situations when non-informed artist might loose their work.

source/blender/blenloader/intern/versioning_common.cc Outdated

						
				@ -50,0 +54,4 @@

				      /* ID_GD_LEGACY (Grease Pencil v2) is now converted to ID_GP (Grease Pencil v3). */

				      return ID_GD_LEGACY;

				    default:

				      return 0;

Sergey Sharybin commented

2024-10-18 17:08:16 +02:00

Should it be ID_LINK_PLACEHOLDER? Otherwise it is a bit unclear for a reader that it is the same thing (return 0 here, and ID_LINK_PLACEHOLDER in the caller).

Should it be `ID_LINK_PLACEHOLDER`? Otherwise it is a bit unclear for a reader that it is the same thing (return 0 here, and `ID_LINK_PLACEHOLDER` in the caller).

Bastien Montagne commented

2024-10-18 18:04:53 +02:00

Indeed.... forgot that one when I did the renaming :|

mont29 marked this conversation as resolved

source/blender/blenloader/intern/versioning_common.hh Outdated

						
				@ -27,0 +38,4 @@

				 * \returns The old, deprecated ID type code if any matches the given `id_code_new` one, otherwise

				 * `ID_LINK_PLACEHOLDER`.

				 */

				short do_versions_new_to_old_idcode_get(const short id_code_new);