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

Sky: Adapt radiance size if AUTOMATIC_MODE resolves to REALTIME #95990

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Sky: Adapt radiance size if AUTOMATIC_MODE resolves to REALTIME #95990

merged 1 commit into from
Sep 4, 2024

Conversation

Breush
Copy link
Contributor

@Breush Breush commented Aug 23, 2024

@Breush Breush requested a review from a team as a code owner August 23, 2024 13:38
@AThousandShips AThousandShips changed the title Sky: Adapted radiance size if AUTOMATIC_MODE resolves to REALTIME - Fixes #76166 Sky: Adapted radiance size if AUTOMATIC_MODE resolves to REALTIME Aug 23, 2024
@AThousandShips AThousandShips changed the title Sky: Adapted radiance size if AUTOMATIC_MODE resolves to REALTIME Sky: Adapt radiance size if AUTOMATIC_MODE resolves to REALTIME Aug 23, 2024
@AThousandShips AThousandShips added bug topic:rendering topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 23, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Aug 23, 2024
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, it works as expected. Code looks good to me.

@akien-mga akien-mga changed the title Sky: Adapt radiance size if AUTOMATIC_MODE resolves to REALTIME Sky: Adapt radiance size if AUTOMATIC_MODE resolves to REALTIME Aug 25, 2024
@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@Breush
Copy link
Contributor Author

Breush commented Aug 25, 2024

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

Thanks, I didn't notice, updated my config.

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.

I don't think this is the correct fix for two reasons:

  1. material_get_data() isn't thread safe. So this change introduces a race condition when using multithreaded rendering
  2. This is hardcoded to only work with the Forward+ and Mobile rendering methods. It will fail when using Compatibility.

I think the correct fix is to not fall back to the realtime mode when the radiance size is not 256. I.e. check for radiance size here:

if (sky_mode == RS::SKY_MODE_AUTOMATIC) {
if (shader_data->uses_time || shader_data->uses_position) {
update_single_frame = true;
sky_mode = RS::SKY_MODE_REALTIME;

and here:

if (sky_mode == RS::SKY_MODE_AUTOMATIC) {
if (shader_data->uses_time || shader_data->uses_position) {
update_single_frame = true;
sky_mode = RS::SKY_MODE_REALTIME;

e.g. if ((shader_data->uses_time || shader_data->uses_position) && sky->radiance_size == 256)

@Breush
Copy link
Contributor Author

Breush commented Sep 2, 2024

I don't think this is the correct fix for two reasons: (...)

Got it. I will update this PR on Wednesday with the suggested fix.

Verified

This commit was signed with the committer’s verified signature.
YeldhamDev Michael Alexsander
@Breush Breush requested a review from clayjohn September 4, 2024 08:55
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!

Note for maintainers, this should be good for cherry picking

@akien-mga akien-mga merged commit 82d7531 into godotengine:master Sep 4, 2024
20 checks passed
@akien-mga
Copy link
Member

akien-mga commented Sep 4, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
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.

Vulkan Forward+: Visual glitch appears in the radiance map at sizes higher or lower than the default
5 participants