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

Restore dirty list for BaseMaterial3D but don't use it on resource loader. #99716

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Nov 26, 2024

Fixes #99576.

Visual updates for Material3D broke in general as I added the ability for materials and shaders to defer the updates according to what @reduz outlined for pipeline pre-compilation.

The dirty list is a bit of a hacky solution to make sure the updates reach in time to the materials on the scene, but we don't have a much better way to do it for architectural reasons. There's sadly no way for these minor material modifications to properly notify the scene geometry that it should request an update again by querying the RID of the Material or the shader.

For the sake of keeping the improvement I added for deferring the shader updates, as it's a huge improvement that allows shaders to be compiled by the resource loader, I used a change that Pedro recently did where it'll skip on adding the material to the dirty list if it's found to be within a resource loader.

While the logic might seem similar at first glance, the big difference is that it does not defer the call using the deferred mechanism but rather it allows the thread that requires the material to load it when necessary.

Requesting a review from @RandomShaper just to make sure this change makes sense.

We should make sure this doesn't break any existing logic with background loaders (demo projects that use background threads on loading screens) and that it still works as intended.

@DarioSamo
Copy link
Contributor Author

I talked with @RandomShaper and I implemented his suggestion of not relying on the getter behavior and going for the new approach that the Material constructor uses.

However, pipeline pre-compilation on meshes on some of my tested scenes have gone down about 10%, resulting in surface pre-compilations instead. It seems the logic for handling the material/shader dependencies is not as exact as it might sound. I've not determined the cause of this reduction yet, and while it's a very small impact, it probably warrants another look to make sure dependencies are sorted out properly for materials.

@DarioSamo DarioSamo force-pushed the material-dirty-updates branch from 18e3d55 to 34a19f0 Compare November 27, 2024 13:37
@DarioSamo
Copy link
Contributor Author

I've restored the original approach the PR takes as it just improves the amount of pipeline pre-compilation in general.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

This is good enough!

However, I may make a follow-up that restores a bit more of the original approach because the lack of pipelines may be rooted elsewhere. I'm testing my hypothesis. Until then...

@Repiteo Repiteo merged commit 70ff57b into godotengine:master Dec 3, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 3, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

Texture filtering does not update properly
4 participants