Revive onion skinning

[RandomShaper]

There may still be little defects (that were possibly present in the original implementation), but this is already big and worth merging to be polished over time.

Fixes #53870.

[SlugFiller]

Tried using this. As soon as I activated onion skin, all I got was this:

ERROR: Condition "!windows.has(p_window)" is true. Returning: false
   at: VulkanContext::window_is_valid_swapchain (drivers\vulkan\vulkan_context.cpp:1746)
ERROR: Condition "!windows.has(p_window)" is true. Returning: false
   at: VulkanContext::window_is_valid_swapchain (drivers\vulkan\vulkan_context.cpp:1746)
ERROR: Condition "!windows.has(p_window)" is true. Returning: false
   at: VulkanContext::window_is_valid_swapchain (drivers\vulkan\vulkan_context.cpp:1746)
ERROR: Condition "!windows.has(p_window)" is true. Returning: false
   at: VulkanContext::window_is_valid_swapchain (drivers\vulkan\vulkan_context.cpp:1746)

[RandomShaper]

I can't reproduce that. Just in case, I've rebased and rebuilt locally and it still works for me.

Can you give a bit more detail? Does the editor freeze or crash, or does it keep working normally, just with onion skinning being futile?

[SlugFiller]

The error appears infinitely. The editor view becomes unresponsive, but it's actually possible to interact with the buttons and close the scene and editor, so I assume only rendering is stopped. But trying to click the onion skin button again to turn it off does not seem to do anything.

Happens in rebased build too.

For what it's worth, I'm running on Windows. Don't know if that's a factor.

[RandomShaper]

I'm on Windows, too, so it must be another thing. What GPU?

[SlugFiller]

Nvidia GeForce RTX 4090

[RandomShaper]

Oh, I was kind of expecting a low VRAM graphics card. I'm clueless at the moment. Summoning @godotengine/production to ask them to give this a test to try to find out what's going on.

[adamscott]

@SlugFiller Could you try to see if this PR works in tandem with this PR? Maybe it's related to CPU/GPU synchronization issues with Vulkan.

[SlugFiller]

Nope, no change. And I even cherry picked both on top of master, so no other merged change had any influence, either.

Interestingly, besides the errors above, I also get this when closing Godot:

ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)
ERROR: Attempted to free invalid ID: 0
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8589)

One theory I can come up with is this:

	DisplayServer::WindowID root_vp_screen = get_tree()->get_root()->get_current_screen();

I have two monitors, and this might be doing something inappropriate

[SlugFiller]

Okay, confirmed this change makes the onion skin not freeze the window:

diff --git a/editor/plugins/animation_player_editor_plugin.cpp b/editor/plugins/animation_player_editor_plugin.cpp
index e674694fec..073f30f5f0 100644
--- a/editor/plugins/animation_player_editor_plugin.cpp
+++ b/editor/plugins/animation_player_editor_plugin.cpp
@@ -1491,7 +1491,7 @@ void AnimationPlayerEditor::_prepare_onion_layers_2() {
 	// Tweak the root viewport to ensure it's rendered before our target.
 	RID root_vp = get_tree()->get_root()->get_viewport_rid();
 	Rect2 root_vp_screen_rect = Rect2(Vector2(), get_tree()->get_root()->get_size());
-	DisplayServer::WindowID root_vp_screen = get_tree()->get_root()->get_current_screen();
+	DisplayServer::WindowID root_vp_screen = DisplayServer::MAIN_WINDOW_ID;//get_tree()->get_root()->get_current_screen();
 	RS::get_singleton()->viewport_attach_to_screen(root_vp, Rect2(), DisplayServer::INVALID_WINDOW_ID);
 	RS::get_singleton()->viewport_set_update_mode(root_vp, RS::VIEWPORT_UPDATE_ALWAYS);

But at the same time, I confirmed the bug I was originally looking for, i.e. no onion skin is visible in this scene:

[gd_scene load_steps=6 format=3 uid="uid://dio1515mgpbg5"]

[sub_resource type="SkeletonModification2DLookAt" id="SkeletonModification2DLookAt_q27bo"]
bone_index = 0
bone2d_node = NodePath("Bone2D")
target_nodepath = NodePath("../Target")

[sub_resource type="SkeletonModificationStack2D" id="SkeletonModificationStack2D_u2c6i"]
enabled = true
modification_count = 1
modifications/0 = SubResource("SkeletonModification2DLookAt_q27bo")

[sub_resource type="Animation" id="Animation_bjlb8"]
tracks/0/type = "value"
tracks/0/imported = false
tracks/0/enabled = true
tracks/0/path = NodePath("Target:position")
tracks/0/interp = 1
tracks/0/loop_wrap = true
tracks/0/keys = {
"times": PackedFloat32Array(0, 1),
"transitions": PackedFloat32Array(1, 1),
"update": 0,
"values": [Vector2(132, 195), Vector2(-130, 186)]
}

[sub_resource type="Animation" id="Animation_oasa5"]
resource_name = "new_animation"
tracks/0/type = "value"
tracks/0/imported = false
tracks/0/enabled = true
tracks/0/path = NodePath("Target:position")
tracks/0/interp = 1
tracks/0/loop_wrap = true
tracks/0/keys = {
"times": PackedFloat32Array(0, 1),
"transitions": PackedFloat32Array(1, 1),
"update": 0,
"values": [Vector2(132, 195), Vector2(-200, 195)]
}

[sub_resource type="AnimationLibrary" id="AnimationLibrary_s1ti5"]
_data = {
"RESET": SubResource("Animation_bjlb8"),
"new_animation": SubResource("Animation_oasa5")
}

[node name="Node2D" type="Node2D"]

[node name="Skeleton2D" type="Skeleton2D" parent="."]
modification_stack = SubResource("SkeletonModificationStack2D_u2c6i")

[node name="Bone2D" type="Bone2D" parent="Skeleton2D"]
position = Vector2(80, 0)
rest = Transform2D(1, 0, 0, 1, 0, 0)
metadata/_local_pose_override_enabled_ = true

[node name="Bone2D2" type="Bone2D" parent="Skeleton2D/Bone2D"]
position = Vector2(89, 0)
rest = Transform2D(1, 0, 0, 1, 0, 0)

[node name="Polygon2D" type="Polygon2D" parent="."]
position = Vector2(-180, 0)
skeleton = NodePath("../Skeleton2D")
polygon = PackedVector2Array(150, -50, 250, -50, 250, 50, 150, 50)
uv = PackedVector2Array(145, -35, 214, -34, 213, 41, 151, 37)
bones = ["Bone2D", PackedFloat32Array(0, 0, 0, 0), "Bone2D/Bone2D2", PackedFloat32Array(1, 1, 1, 1)]

[node name="Target" type="Node2D" parent="."]
position = Vector2(132, 195)

[node name="AnimationPlayer" type="AnimationPlayer" parent="."]
libraries = {
"": SubResource("AnimationLibrary_s1ti5")
}

The reason stems from the fact that SkeletonModificationStack2D runs on _process, and the onion skin generator does not call OS::get_singleton()->get_main_loop()->process between viewport renders.

[RandomShaper]

@SlugFiller, I've been able to make a very comprehensive fix thanks to your feedback. Moreover, involved cases such as the one in your test scene now work. Onion skinning now uses a brute force approach to update the animation at each step instead of being anaytical in which calls are needed to make exactly to have, e.g., bones updated.

Also, thanks to having to check the code more thoroughly, I've been able to remove the issue of onion layers being a frame late.

Please give it another test. If it works for you, I believe this is ready for production (maybe with a disclaimer about it being experimental for now in the release notes).

[SlugFiller]

I think it's missing a final _process (or maybe even a seek) call before drawing the present, because the present seems to draw as after the last future, and remains there even after turning off onion skin
onion_bug.webm

I'm also noticing some weird misalignment with the position of the "Center View" button and scroll bars in the onion renderings (arguably, they shouldn't appear at all in the onion layers, but if they do, they should be in the right spot)

But I will note that it works surprisingly well, even on non-builtin modifications. When it works perfect, I'll try to give the code a quick browse too.

(Should probably test in 3D too. But Don't have many 3D animations that are not in Godot 3)

[RandomShaper]

@SlugFiller, can you please check if replacing
player->seek(onion.temp.anim_player_position, false);
with
player->seek(onion.temp.anim_player_position, true);
fixes the first issue you mention?

[SlugFiller]

@RandomShaper Perfect

There's still the weird part where the "center view" button is off in the onions (scroll bars too). But that's just a nitpick. This is working great.

P.S. It might be an issue with how the blending is done, but it sort of looks like the onions are contributing more to the final color than the current frame, making them look "on top" of it. Or maybe it's just down to the step count? It doesn't matter much, because picking "differences only" already makes the current frame be on top of the others. But is there a way to adjust it slightly?

[RandomShaper]

@SlugFiller, I can't express how much I appreciate your testing and feedback. In the end, I've left both methods of restoring the state of the properties affected by the animation (the original one plus the seek with true), including a note about why both need to cooperate in some cases.

Regarding the visuals, alpha could be handled better indeed and also it's true that some UI stuff like scrollbars or UI icons don't play very well with onion skinning. And, well, that's something I would like to leave for a future iteration, not even necessarilly by me. I think this already brings back a feature in a very functional state and now I have to keep focusing in other duties. (I'll be watching for bug fixes, though.)

[clayjohn]

Looks fine to me. I trust the local reviews done by others.

The rendering server code looks good. I am less confident on the changes to the editor code as that is outside my area of expertise.

[RandomShaper]

Rebased and also added the change that a reset animation must be created (see #80813 (comment) and below). Since it's now based on a reset animation, it will only work correctly with tracks for which there's a corresponding well-setup reset track. I still think it's good enough, but I wish I (or other) can make a future iteration bringing back the former user-friendliness.

[HeadClot]

Does this work for 2D as well as 3D?

[RandomShaper]

Does this work for 2D as well as 3D?

Yes.

If we had 4D (with the 4th not being time), it would work as well. 😃

[akien-mga]

Thanks!