Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix errors when freeing GPUParticles #82431

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Sep 27, 2023

The fundamental reason for the first linked bug seems to be that when a GPUParticles2D or GPUParticles3D is freed and destroyed, they eventually call ParticlesStorage::particles_free(). This in turn has a somewhat unusual call to ParticlesStorage::update_particles(). So deleting a single particle system can force update all other currently processing particles systems. I looked around and I think the main reason for this call is that this "update all" has a side effect of removing the currently deleted particle system from the particle_update_list so that it doesn't point to invalid memory after the particle system is freed soon after. However, there might be more subtler reason too, but I didn't notice anything suspicious. Indirectly sending the Dependency::DEPENDENCY_CHANGED_AABB is in theory something that could matter, but I don't think that is needed as the particle system is being deleted anyway. I replaced the simple update list management with a SelfList, so there is now a more standard mechanism for tracking updates and we avoid the wasteful updates.

Now, this update call can also invalidate some particle system material uniform sets. Normally, this doesn't seem to be a problem because they will be re-created when needed, but on some occasions this can happen during a particle system destructor. When this matters depends on many variables, including active particle system count, Material "Local to Scene" flag and even where and when the queue_free() is called as the event flushing and destructor call can happen in several places during the main loop iteration. A different particle system might be updated immediately, but if the material uniform set was invalidated (here is where that "Local to Scene" flag seems to matter) and has not been re-created yet the update will cause the bug and error message with potential for visual glitches. Every time you see that error message in the MRP, the debugger stack trace will point to a particle system queued destructor being run and calling update_particles().

Another hint that calling update_particles() at random times is not a good idea is the comment in RenderingServerDefault::draw(), suggesting that the update order is important:

RSG::particles_storage->update_particles(); //need to be done after instances are updated (colliders and particle transforms), and colliders are rendered

The exact circumstances where this bug can happen are pretty complex and I don't claim I understand them all, but simply avoiding these potentially troublesome updates in ParticlesStorage::particles_free() seems to fix the fundamental problem. Another alternative one-liner fix is to simply change the branch condition in ParticlesStorage::_particles_process()

ParticleProcessMaterialData *m = static_cast<ParticleProcessMaterialData *>(material_storage->material_get_data(p_particles->process_material, MaterialStorage::SHADER_TYPE_PARTICLES));
if (!m) {
    m = static_cast<ParticleProcessMaterialData *>(material_storage->material_get_data(particles_shader.default_material, MaterialStorage::SHADER_TYPE_PARTICLES));
}

to

if (!m || m->uniform_set.is_null()) {

However, I think changing that might just hide the issue and could still cause some visual glitches. Still, in case more bug reports like these appear in the future, adding that extra check might be useful.

Again, this is a pretty tricky issue, so getting another opinion of this will be useful.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master d31794c), it works as expected.

I don't know if the code approach is the ideal one (my knowledge of particle RID stuff is limited), so a second review may help.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me and I trust the testing that has been done by others.

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 11, 2023
@akien-mga akien-mga merged commit b137180 into godotengine:master Oct 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment