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 3D materials not updating on texture invalidation #54370

Closed
wants to merge 1 commit into from

Conversation

briansemrau
Copy link
Contributor

@briansemrau briansemrau commented Oct 29, 2021

See better implementation: #54489


Fixes #51361
Fixes #53206
Fixes #53089
Should fix #42821
Might fix #50296
Might fix #52505
Might fix #45746

It's possible there are other issues related to/fixed by this.

Might be relevant for #50327.

Supersedes #52528

2021-10-29.10-18-00.mp4

Explanation of the issue here:
#51361 (comment)

This is essentially finishing what #50269 set out to do. Implementation derived from:

if (!RD::get_singleton()->uniform_set_is_valid(md->uniform_set)) {
// uniform set may be gone because a dependency was erased. In this case, it will happen
// if a texture is deleted, so just re-create it.
storage->material_force_update_textures(material, RendererStorageRD::SHADER_TYPE_2D);
}

@clayjohn
Copy link
Member

I think we will need to add the same fix to particle shaders, sky shaders, and fog shaders.

@briansemrau
Copy link
Contributor Author

I'll hunt down all missing spots. I noticed I've missed a few.

@briansemrau
Copy link
Contributor Author

briansemrau commented Oct 29, 2021

I checked all usage of RendererStorage::material_get_data, so this should cover everything.

@clayjohn
Copy link
Member

This is an area of the code I am unfamiliar with, but it looks like reduz wrapped the calls in a check to ensure that material_force_update_texture was only ever called once per frame

if (md->last_frame != RendererCompositorRD::singleton->get_frame_number()) {
md->last_frame = RendererCompositorRD::singleton->get_frame_number();
if (!RD::get_singleton()->uniform_set_is_valid(md->uniform_set)) {
// uniform set may be gone because a dependency was erased. In this case, it will happen
// if a texture is deleted, so just re-create it.
storage->material_force_update_textures(material, RendererStorageRD::SHADER_TYPE_2D);
}
}

I'm wondering if it should be the same for the other calls

@briansemrau
Copy link
Contributor Author

briansemrau commented Oct 30, 2021

Yup, seems those checks were intended design, as last_frame is unused in a lot of materials. Updated.
The uniform set should only be invalid once per frame, so maybe this isn't a big deal in the end. The only code run every material use would be RID_Alloc::owns, which doesn't look expensive, but redundant nonetheless.

@akien-mga akien-mga requested a review from reduz November 1, 2021 09:30
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.

Looks good to me! This is something that reduz should have the final say on though.

@briansemrau
Copy link
Contributor Author

briansemrau commented Nov 1, 2021

Somehow didn't consider that there's a lot of code repetition here. If this is the preferred solution, it would be good for this to have its own function.

@akien-mga
Copy link
Member

Superseded by #54489.

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