Broken rendering with CPUParticles after #79833

[akien-mga]

Tested versions

Regression in 4.3, bisected to #79833 / ec6518d.

System information

Fedora Linux 39 (KDE Plasma) - Wayland - Vulkan (Forward+) - dedicated AMD Radeon RX 7600M XT (RADV NAVI33) () - AMD Ryzen 7 7840HS w/ Radeon 780M Graphics (16 Threads)

Issue description

There's a pretty dramatic rendering issue in https://github.com/godotengine/tps-demo/ introduced recently.

I bisected it to ec6518d, CC @puchik.
Reverting that commit locally fixes the issue.

Here's how it looks like as a screenshot:

Main menu:

In game:

Epilepsy warning. In motion, it's dramatically flickering.

It might be OS/GPU specific. Here's my config:

Fedora Linux 39 (KDE Plasma) - Wayland - Vulkan (Forward+) - dedicated AMD Radeon RX 7600M XT (RADV NAVI33) () - AMD Ryzen 7 7840HS w/ Radeon 780M Graphics (16 Threads)

Steps to reproduce

• Run https://github.com/godotengine/tps-demo/ with current master

Minimal reproduction project (MRP)

https://github.com/godotengine/tps-demo/

[DarioSamo]

The issue itself is pretty random for me so it's very hard to determine if something has actually fixed it, but I've been unable to reproduce it after taking out this line.

godot/scene/3d/cpu_particles_3d.cpp

Line 1687 in fb10e67

set_visibility_aabb(AABB(Vector3(-4, -4, -4), Vector3(8, 8, 8)));

Following the logic it seems like this calls the following:

godot/scene/3d/cpu_particles_3d.cpp

Lines 104 to 108 in fb10e67

	void CPUParticles3D::set_visibility_aabb(const AABB &p_aabb) {
	RS::get_singleton()->multimesh_set_custom_aabb(multimesh, p_aabb);
	visibility_aabb = p_aabb;
	update_gizmos();
	}

Which means RenderingServer will be stuck with a custom AABB assigned to the particle that overrides the actual AABB that gets updated.

A break point confirms this path is getting hit even if custom AABBs were never set:

Which is pretty bad because updating dirty multimeshes is right after this new condition:

godot/servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp

Lines 2036 to 2047 in fb10e67

	AABB MeshStorage::multimesh_get_aabb(RID p_multimesh) const {
	MultiMesh *multimesh = multimesh_owner.get_or_null(p_multimesh);
	ERR_FAIL_NULL_V(multimesh, AABB());
	if (multimesh->custom_aabb != AABB()) {
	return multimesh->custom_aabb;
	}
	if (multimesh->aabb_dirty) {
	const_cast<MeshStorage *>(this)->_update_dirty_multimeshes();
	}
	return multimesh->aabb;
	}

There's still a chance this isn't the fix but it seems to fit together at least.

[puchik]

I had a moment to give this a quick test, very weird. I'm not seeing this issue on Windows 10 19045, i7-7700HQ, GTX 1060, Vulkan 1.3.260 Forward+ but between current release and current master I cannot move and get error spam like this (it goes on for much longer):

Possibly unrelated, but either way seems like no rendering/CPU particle issues?

I can make CPUParticles in the editor as well with no obvious crashes.

[akien-mga]

@puchik The AnimationMixer was fixed already, you need to rebase on latest master.

There's no crash, just bad AABBs which break the rendering.

I confirm that removing

set_visibility_aabb(AABB(Vector3(-4, -4, -4), Vector3(8, 8, 8))); 

fixes the issue for me too.

Which makes sense as the checks for whether to use the custom_aabb all check against AABB() (i.e. an empty / zero AABB).

[DarioSamo]

Does this attribute get serialized? I wonder if there's a chance it got set for any projects that saved particles with the commit since it was introduced.

EDIT: Apparently not since it's at its default value luckily. But I can confirm it does show up on this default value in the editor.

[puchik]

I managed to reproduce it on another system by manually entering the main menu level, enabling emitting on the particles, and moving the camera around. Removing the line fixes the issue for me, as well. Given the custom AABB is checked for null, it does make sense for it to not have an initial value, no? It will calculate one dynamically if it is, anyway.

[DarioSamo]

I managed to reproduce it on another system by manually entering the main menu level, enabling emitting on the particles, and moving the camera around. Removing the line fixes the issue for me, as well. Given the custom AABB is checked for null, it does make sense for it to not have an initial value, no? It will calculate one dynamically if it is, anyway.

That sounds correct to me, but I prefer delegating to you to do the fix on this one so you get a chance to review if the particle editor works correctly... I'd also review the behavior of whether you want to skip updating dirty multimeshes or not based on the get if it has a custom AABB. My initial feeling is that you might want to process the dirty AABB for both cases considering it uploads the data required for rendering on that step.

[puchik]

Particle editor seems OK. Same as TPS demo. Best to keep in mind the issue was less prevalent for me, though.

On the second point, was avoiding updating the AABBs not one of the main benefits of that change? Unless I'm missing something or misunderstanding. CC @clayjohn

Also, would that be a separate question/issue? Should I push the CPUParticle fix for this for now?

[DarioSamo]

Also, would that be a separate question/issue? Should I push the CPUParticle fix for this for now?

As it currently breaks rendering in any project that uses particles I'd say it's a high priority yeah, even if we opt for the quick fix and look into it further later if there happens to be something better. Right now I'm fairly sure this is wrong anyway as it'll show up in the editor with the default value for the AABB even if you've never edited it.