Cycles: Fix Principled BSDF versioning for keyframes and drivers #114300

Lukas Stockner · 2023-10-31T05:44:12+01:00

Lukas Stockner commented

2023-10-31 05:44:12 +01:00

This is a last-minute patch to address #113898.

The base logic is there, but it has some remaining issues:

Drivers/keyframes don't transfer to the extra nodes in automatic conversion setups (probably fine)
Drivers/keyframes on Sheen Tint/Specular Tint only apply to the R channel, since they changed from float to color (probably fine)
Drivers/keyframes for removed sockets (Transmission Roughness, Subsurface Color) stay in the file. Not sure how to clean them up properly, for now I set their index to -1 so that they become invalid instead of affecting an unrelated socket.
The versioning-test.blend from the linked bug discussion crashes on opening. No idea why, to be honest. My own test file works.

I'm at my limit for knowledge of the animdata system - considering the urgency here, it'd be great if @brecht (or someone else) could pick this up.

This is a last-minute patch to address #113898. The base logic is there, but it has some remaining issues: - Drivers/keyframes don't transfer to the extra nodes in automatic conversion setups (probably fine) - Drivers/keyframes on Sheen Tint/Specular Tint only apply to the R channel, since they changed from float to color (probably fine) - Drivers/keyframes for removed sockets (Transmission Roughness, Subsurface Color) stay in the file. Not sure how to clean them up properly, for now I set their index to -1 so that they become invalid instead of affecting an unrelated socket. - The `versioning-test.blend` from the linked bug discussion crashes on opening. No idea why, to be honest. My own test file works. I'm at my limit for knowledge of the animdata system - considering the urgency here, it'd be great if @brecht (or someone else) could pick this up.

❤️ 1

Lukas Stockner added this to the 4.0 milestone 2023-10-31 05:44:12 +01:00

Brecht Van Lommel was assigned by Lukas Stockner

2023-10-31 05:44:12 +01:00

Lukas Stockner force-pushed fix-principled-animdata from 97dc0de163 to 4f54ad4f5e

2023-11-01 05:29:32 +01:00

Compare

Lukas Stockner changed title from ~~WIP: Cycles: Fix Principled BSDF versioning for keyframes and drivers~~ to Cycles: Fix Principled BSDF versioning for keyframes and drivers

2023-11-01 05:29:36 +01:00

Lukas Stockner commented

2023-11-01 05:30:19 +01:00

The crash was solved by moving the logic to do_versions_after_linking_400.

Also, I've added code to remove animdata for the removed sockets, it turned out to be easier than expected.

The crash was solved by moving the logic to `do_versions_after_linking_400`. Also, I've added code to remove animdata for the removed sockets, it turned out to be easier than expected.

Brecht Van Lommel requested changes 2023-11-01 15:15:49 +01:00

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

						
				@ -245,0 +260,4 @@

				      BKE_animdata_fix_paths_remove(id, (prefix + "[18]").c_str());

				      BKE_animdata_fix_paths_remove(id, (prefix + "[3]").c_str());

				    };

				    BKE_animdata_main_cb(bmain, remove_old_inputs, &prefix);

Brecht Van Lommel commented

2023-11-01 15:15:46 +01:00

This looks like it loops over all node trees in the file, not just this one?

Lukas Stockner commented

2023-11-01 18:56:34 +01:00

Yes, seems like it does. However, as far as I can tell, BKE_animdata_fix_paths_rename_all_ex does the same - the nodetree ID that is being passed into it is only used in one bone-specific check in drivers_path_rename_fix, which doesn't apply here. And that function is used for existing node versioning.

For the test file from the report, everything actually also works if we don't use BKE_animdata_fix_paths_rename_all_ex and BKE_animdata_main_cb for iteration, and instead just call BKE_animdata_fix_paths_remove/BKE_animdata_fix_paths_rename directly on the nodetree.
But I guess there is some reason to have the iteration in the original code? Not sure how all the ownership works here.

I'll update the code to skip the iteration for now, since it still seems to work.

Yes, seems like it does. However, as far as I can tell, `BKE_animdata_fix_paths_rename_all_ex` does the same - the nodetree ID that is being passed into it is only used in one bone-specific check in `drivers_path_rename_fix`, which doesn't apply here. And that function is used for existing node versioning. For the test file from the report, everything actually also works if we don't use `BKE_animdata_fix_paths_rename_all_ex` and `BKE_animdata_main_cb` for iteration, and instead just call `BKE_animdata_fix_paths_remove`/`BKE_animdata_fix_paths_rename` directly on the nodetree. But I guess there is some reason to have the iteration in the original code? Not sure how all the ownership works here. I'll update the code to skip the iteration for now, since it still seems to work.