Duplicated Multimesh breaks when TAA is active

JoanPotatoes2021 commented 1 year ago

Godot version

v4.0.beta2.official [f8745f2f7]

System information

Windows 10, Vulkan, Nvidia Geforce GTX 1050 TI

Issue description

It seems that TAA breaks when duplicated multimeshs are visible, see the MRP, if only one instance of the multimesh is visible it works properly, however when TAA is active and the multimeshs duplicated are visible, reload the project to continue testing,

TaaBreaksMultimesh01b

Steps to reproduce

The MRP was created with the follow steps:

1 - Import multiple meshes as a multimesh using a post-import script, (included in MRP) 2 - Duplicate the post-imported gltf in the scene now as multimesh, 3 - Enable TAA in Antialias render in project settings.

To test the issue with the MRP, just open the MRP and activate the TAA in the project settings, should spam with errors the output window.

Minimal reproduction project

Multimesh Flicker Godot4 Beta2.zip

MGilleronFJ commented 1 year ago

The relation between multimesh and TAA sounds very surprising. Might be related: https://github.com/godotengine/godot/issues/56357 (although no TAA)

JoanPotatoes2021 commented 1 year ago

I had this issue that generated this report during import, everytime I tried to re-import the gltfs with the MRP post-import script to generate multimeshes, only when disabling TAA it imports ok, where as if TAA was enabled the transforms of the multimeshs were all messed up, see gif below.

TaaBreaksMultimesh01c 1 - The gltf file, 2 - Disabled TAA, 3 - Imports ok, 4 - TAA enabled, 5 - After re-import, the transforms of the multimesh glitch.

The last multimesh has distorted transforms, the weird part is if I keep re-importing the distortions keep changing, and sometimes it doesn't even generate the transforms for the multimeshes, the multimesh buffer array keeps everything as 0.0, and no errors popped up,

I am also not running any toolscripts.

Swarkin commented 1 year ago

Those messed up transforms happen without TAA as well, in my project they are stretched in the editor and generate an error every frame:

I manually created the multimeshes. No matter what setting I change, it always displays them like this. It works fine in game. drivers/vulkan/rendering_device_vulkan.cpp:7177 - Condition "!uniform_set" is true.

However, the addon MultiMeshScatter is able to create multimeshes that properly work inside the editor:

Calinou commented 1 year ago

@Swarkin This is likely an unrelated issue, see https://github.com/godotengine/godot/issues/56357.

Calinou commented 1 year ago

I can confirm this on 4.1.dev 31eccb550 (Linux, GeForce RTX 4090 with NVIDIA 525.89.02).

TAA disabled

TAA enabled

This is spammed in the console when running the project:

ERROR: Condition "!uniform_set" is true.
at: draw_list_bind_uniform_set (drivers/vulkan/rendering_device_vulkan.cpp:7199)

DezoEsper commented 1 year ago

Godot v4.0 rc6 win64: TAA used - when MultiMeshInstance3D is cloned on scene I get a glitch effect and spam error log

E 0:00:50:0746 draw_list_bind_uniform_set: Condition "!uniform_set" is true.<Source Code C++>drivers/vulkan/rendering_device_vulkan.cpp:7199 @ draw_list_bind_uniform_set()

One MultiMeshInstance3D doesn't appear drawn on the scene, but two different MultiMeshInstance3D are drawn fine.

smix8 commented 1 year ago

On windows on recent master with TAA enabled and shared MultiMeshes I get the same uniform set error spam. When I save the scene in question I get the error spam that persists forever and requires an Editor restart to go away.

When I set the MultiMesh resources to local_to_scene I don't get the error spam. Instead I get an immediat crash with this backtrace on a dev build when the scene with the multimesh is instantiated.

Note that everything including the rendering is set to run singlethreaded in the ProjectSettings.

CrashHandlerException: Program crashed Engine version: Godot Engine v4.1.dev.custom_build (055ee1276f097727d1c0ba3c1c25a12981c20bfa) Dumping the backtrace. Please include this when reporting the bug to the project developer. [0] <couldn't map PC to fn name> [1] <couldn't map PC to fn name> [2] <couldn't map PC to fn name> [3] RenderingDeviceVulkan::draw_list_draw (H:\files\github\godot\godot_4.x_fork\drivers\vulkan\rendering_device_vulkan.cpp:7390) [4] RendererSceneRenderImplementation::RenderForwardClustered::_render_list_template<3,0> (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_rd\forward_clustered\render_forward_clustered.cpp:514) [5] RendererSceneRenderImplementation::RenderForwardClustered::_render_list (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_rd\forward_clustered\render_forward_clustered.cpp:558) [6] RendererSceneRenderImplementation::RenderForwardClustered::_render_list_with_threads (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_rd\forward_clustered\render_forward_clustered.cpp:597) [7] RendererSceneRenderImplementation::RenderForwardClustered::_render_scene (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_rd\forward_clustered\render_forward_clustered.cpp:1853) [8] RendererSceneRenderRD::render_scene (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_rd\renderer_scene_render_rd.cpp:1036) [9] RendererSceneCull::_render_scene (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_scene_cull.cpp:3319) [10] RendererSceneCull::render_camera (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_scene_cull.cpp:2590) [11] RendererViewport::_draw_3d (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_viewport.cpp:214) [12] RendererViewport::_draw_viewport (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_viewport.cpp:279) [13] RendererViewport::draw_viewports (H:\files\github\godot\godot_4.x_fork\servers\rendering\renderer_viewport.cpp:729) [14] RenderingServerDefault::_draw (H:\files\github\godot\godot_4.x_fork\servers\rendering\rendering_server_default.cpp:92) [15] RenderingServerDefault::draw (H:\files\github\godot\godot_4.x_fork\servers\rendering\rendering_server_default.cpp:397) [16] Main::iteration (H:\files\github\godot\godot_4.x_fork\main\main.cpp:3191) [17] OS_Windows::run (H:\files\github\godot\godot_4.x_fork\platform\windows\os_windows.cpp:1300) [18] widechar_main (H:\files\github\godot\godot_4.x_fork\platform\windows\godot_windows.cpp:181) [19] _main (H:\files\github\godot\godot_4.x_fork\platform\windows\godot_windows.cpp:203) [20] main (H:\files\github\godot\godot_4.x_fork\platform\windows\godot_windows.cpp:217) [21] WinMain (H:\files\github\godot\godot_4.x_fork\platform\windows\godot_windows.cpp:231) [22] __scrt_common_main_seh (d:\a01_work\12\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288) [23] <couldn't map PC to fn name> -- END OF BACKTRACE --

DarioSamo commented 1 year ago

It seems the link behind TAA breaking this and the Multimesh error spam is this particular bit of code. Particularly enabling the use of motion vectors on the color pass.

https://github.com/godotengine/godot/blob/cc6a60913aaba2e41c87741ecc5a6a37835320a4/servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp#L1014C63-L1014C63

if (p_color_pass_flags & COLOR_PASS_FLAG_MOTION_VECTORS) {
    if ((flags & (INSTANCE_DATA_FLAG_MULTIMESH | INSTANCE_DATA_FLAG_PARTICLES)) == INSTANCE_DATA_FLAG_MULTIMESH && RendererRD::MeshStorage::get_singleton()->_multimesh_enable_motion_vectors(inst->data->base)) {
        inst->transforms_uniform_set = mesh_storage->multimesh_get_3d_uniform_set(inst->data->base, scene_shader.default_shader_rd, TRANSFORMS_UNIFORM_SET);
    }
}

The instance already has a valid transforms_uniform_set, but this code replaces the existing one. However, it seems the render list still references the old RID when it reaches the render list, resulting in an invalid reference (is it possible the uniform set gets trashed?).

Removing this code in particular makes the error go away, but I'm not sure what the implications are or if it breaks anything else.

@JFonS is shown as the last author of that particular bit of code along with a few commit messages explaining the reasoning behind it. Maybe it's worth taking a second look at this fix in this particular issue as to why it's replacing a valid uniform set and causing the rendering error?

DarioSamo commented 1 year ago

After further investigation I'd have to say there's a bit of a deeper problem in how multimesh handles the update of the buffer that makes this quite hard to fix.

My understanding of the problem so far is:

Multimesh requires a transforms buffer that uses dynamic indexing to work.
To support motion vectors on this, the transforms buffer requires to be double its size. This is adjusted dynamically by the renderer when it's time to fill the render list.
An additional offset is tracked to understand where in the transforms buffer the current and previous transform are located.

However, when it comes to where this is handled, it seems the renderer itself will forcefully update the mesh storage to duplicate the buffer size, and therefore invalidating the previous buffer.

This poses a few problems in the case of duplicated instances, as only the first instance where this happens will determine it's necessary to create a new uniform and the rest will have the old resource bound. This is why the duplicated versions of the multimesh fail to bind the uniform correctly the first time.

A first temporary solution would be to track that this is necessary for all instances that invalidate the multimesh buffer and re-recreate them, fixing this particular bug (which would require creating some sort of invalidation set during the render list creation, which seems ugly). Another one could be to move this validation to _update_dirty_geometry_instances, which while it doesn't have access to know whether it needs motion vectors or not, its parent function does. This one is a design decision that might need to be validated by someone else.

I'll attempt the temporary fix, but there's some things that don't sit right with me with the current solution as it seems a bit messy for the renderer to be able to change the mesh storage instead of just requesting a separate buffer.

EDIT: _update_dirty_geometry_instances would sadly not work as intended, as the multimesh instance would not be marked as dirty just because TAA is enabled.

godotengine / godot