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 several Material texture parameter updates #84303

Merged

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Nov 1, 2023

Fixes the issues when assigning or initializing a null/default texture to certain material properties doesn't properly update the material state. This can happen in normal use when updating textures in code, but usually this is more noticeable when duplicating a material either explicitly by calling duplicate() or indirectly during instantiation if the material has Local To Scene enabled. This bug causes certain empty textures in a material to be initialized incorrectly. Seems like at least emission, back light and height mapping textures are most affected as they use hint_default_black fallback.

I also noticed possible issues in particle process material, so this fix may also affect particle system behavior in certain situations. In normal materials this bug is usually obvious and can be worked around as discussed in the issue reports, but with particle systems the change in behavior might not be immediately clear.

The basic problem is that calling RenderingServer::material_set_param() with a Variant::Type::NIL parameter value will result in different behavior compared to using a zero/null RID. Only a NIL Variant will clear the internal material parameter inside the MaterialStorage:

if (p_value.get_type() == Variant::NIL) {
material->params.erase(p_param);
} else {
ERR_FAIL_COND(p_value.get_type() == Variant::OBJECT); //object not allowed
material->params[p_param] = p_value;
}

Many places already correctly use the NIL Variant, so I changed all obvious places I could find in this PR. Another alternative is to change MaterialStorage::material_set_param() to clear the material (texture) property also when using an empty RID. However, as that API is exposed to scripting it would change existing behavior, although I doubt many people are relying on that behavior and if they are their code is probably buggy in a similar way.

@bitsawer bitsawer added this to the 4.2 milestone Nov 1, 2023
@bitsawer bitsawer requested a review from a team November 1, 2023 12:22
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 great! Seems like we caught this for SkyMaterials a long time ago and just never checked the other places where this pattern is used (#75469 (review))

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Nov 1, 2023
@akien-mga akien-mga merged commit c6c4728 into godotengine:master Nov 1, 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 Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4. (Omitting some methods introduced by PRs which were not and will not be cherry-picked, such as the particles refactor).

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