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

Add Blend Distance property to ReflectionProbe #99958

Merged

Conversation

lander-vr
Copy link
Contributor

@lander-vr lander-vr commented Dec 3, 2024

Supersedes #61416

This PR adds the "Blend Distance" property to reflection probes. This parameter is exposed to offer artists control over how quickly a reflection probe blends into the scene.
Some context on why this property is necessary: #61416 (comment)

This PR builds on @Calinou's initial work, however I've entirely reworked how the blend parameter is calculated and smoothed.
This is for two reasons:

  • The blend is now calculated for each axis of the probe separately. This is necessary to keep an even blend-distance on each side of the probe bounds. In other words, this avoids the blending being warped based on the proportions of the AABB.
  • The blend smoothing before had an issue where it would strangely reduce the strength inside the probe.

Blend Distance is a float representing the length of the blend in meters, with a default range of 0 to 8, or greater.

I've added an interior wireframe to display the extends of the blend-distance within the AABB of the main probe. This makes it clear at what point the blending stops, and where the probe emits "full strength". (For the sake of presentation, I commented out the lines between the corners and center_offset for this GIF, this would be the case if #99920 gets merged)

24_12_03_12_54__godot windows editor x86_64_b89yussjEj

Wall test scene, replication of the example scenario I gave in the context comment:
24_12_03_01_36__godot windows editor x86_64_w4N2WPk8jF

Comparison of the same scene to 4.3:

4.3 - unadjusted probes 4.3 - adjusted probes This PR
image image image

Unadjusted probes have the same bounds as in the PR.
Adjusted probes have enlarged bounds, which is why the wall in the adjusted probes get stronger reflections. However, since this needs to happen on both sides of the wall to accommodate for the view from that side, you get bleeding.
Notice how despite that adjustment and being well within the bounds of the probe, the wall is still significantly darker due to not receiving full reflection strength.

@lander-vr lander-vr requested review from a team as code owners December 3, 2024 12:05
@lander-vr lander-vr force-pushed the reflection-probe-blend-property branch from b9ac141 to 755e323 Compare December 3, 2024 12:37
@lander-vr lander-vr requested a review from a team as a code owner December 3, 2024 12:37
@jcostello
Copy link
Contributor

Looks awesome. I notice that you can blend above the bounds of the probes. Is that correct? I tried and I got nice results but I wonder why above the bounds

@lander-vr
Copy link
Contributor Author

lander-vr commented Dec 3, 2024

Looks awesome. I notice that you can blend above the bounds of the probes. Is that correct? I tried and I got nice results but I wonder why above the bounds

I don't think limiting the property based on the size of the box really gives any UX improvements, if anything it could lead to unwanted changes of that value if the user accidentally makes the bounding box smaller than the max blending distance for those bounds. It would also feel needlessly restrictive, in my opinion. The actual blending is min(reflections.data[ref_index].blend_distance, box_extents[i]), so the blending itself is clamped based on the extents of the probe: even though you can drag the blend distance further, you shouldn't actually see a visual change if your probe is too small. You can however then increase the bounds of your probe and the internal bounding box will appear again keeping that blending distance. I think this feels a bit more flexible and nicer.

24_12_03_15_02__godot.windows.editor.x86_64_JzzmqvKW5A.mp4

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 in Forward+ and Mobile. This new blending makes a lot more physical sense overall.

Testing project: test_reflection_probes_mirage_palace.zip

master This PR
Screenshot_20241203_163807 png webp Screenshot_20241203_163755 png webp

Additionally, this allows for better fake area lights using bright custom ambient colors:

image

image

In Compatibility, I get a shader compilation error and the scene looks fully white. This doesn't occur in master.

ERROR: SceneShaderGLES3: Fragment shader compilation failed:
0(1085) : error C1503: undefined variable "reflections"
0(1085) : error C1503: undefined variable "ref_index"
0(1087) : error C1503: undefined variable "reflections"
0(1087) : error C1503: undefined variable "ref_index"

@jcostello
Copy link
Contributor

Looks awesome. I notice that you can blend above the bounds of the probes. Is that correct? I tried and I got nice results but I wonder why above the bounds

I don't think limiting the property based on the size of the box really gives any UX improvements, if anything it could lead to unwanted changes of that value if the user accidentally makes the bounding box smaller than the max blending distance for those bounds. It would also feel needlessly restrictive, in my opinion. The actual blending is min(reflections.data[ref_index].blend_distance, box_extents[i]), so the blending itself is clamped based on the extents of the probe: even though you can drag the blend distance further, you shouldn't actually see a visual change if your probe is too small. You can however then increase the bounds of your probe and the internal bounding box will appear again keeping that blending distance. I think this feels a bit more flexible and nicer.

The only visual change I see using blender distance further is that you control how much of the sky blends. But maybe we can have a separate slider to control how much the sky blends when using inside=false

@lander-vr
Copy link
Contributor Author

lander-vr commented Dec 3, 2024

The only visual change I see using blender distance further is that you control how much of the sky blends. But maybe we can have a separate slider to control how much the sky blends when using inside=false

I'm not entirely sure I understand what you mean. Is this not the behavior you would expect?
24_12_03_17_34__godot windows editor x86_64_NWy40nWQwd

Keep in mind that this PR is not really supposed to solve any of the current sky blending issues, this PR just deals with how things blend, not which things. To fix sky blending we need a priority system and sky visibility masks for reflection probes.

@lander-vr lander-vr force-pushed the reflection-probe-blend-property branch from b9ba4b8 to 3d06b31 Compare December 3, 2024 17:05
@lander-vr lander-vr requested a review from Calinou December 3, 2024 17:22
@lander-vr
Copy link
Contributor Author

In Compatibility, I get a shader compilation error and the scene looks fully white. This doesn't occur in master.

Should be fixed now! :)

@lander-vr lander-vr force-pushed the reflection-probe-blend-property branch from 3d06b31 to 2945d91 Compare December 3, 2024 17:35
@jcostello
Copy link
Contributor

I'm not entirely sure I understand what you mean. Is this not the behavior you would expect? !

Maybe its the expected behavior but it feel strange to go beyond the limits of the probes. Like in my mind I dont know what it means to blend further that. Like I said, the only thing I notice was the sky.

To me, if the probe is only 1x1x1m, the maximum blend distance should be .5m. But I like better the unity approach that is with a float value from 0 to 1. Meaning 0 not blending at all and 1 the maximum distance from the center of the probe. Does it make sense in this case?

@lander-vr
Copy link
Contributor Author

Maybe its the expected behavior but it feel strange to go beyond the limits of the probes.

I'll add a bit more context in the tooltip.

Like I said, the only thing I notice was the sky.

This is strange, I cannot reproduce this. Once the blend distance reaches it's internal limit, there's no visual change for me when going far beyond that value. Looking at it pragmatically though, I don't think it matters too much. In normal use cases the blend range will only be a small part of the box.

24_12_03_19_10__godot windows editor x86_64_r8nNz5rVwF

To me, if the probe is only 1x1x1m, the maximum blend distance should be .5m. But I like better the unity approach that is with a float value from 0 to 1. Meaning 0 not blending at all and 1 the maximum distance from the center of the probe. Does it make sense in this case?

I think this is mostly a case of getting used to this behavior if you're unfamiliar with it. In the end they're not vastly different.
A % value of the bounds is more abstract in my opinion (For example % of which side?), and causes varying blend distances with different sizes probes.
To me a distance makes more sense, it's more predictable. A meter is a meter, regardless the size of the probe. The probe can have dimensions of 500m x 10m x 10m, but when the blend distance is set to 1 meter, it will still be 1 meter of equal blend on all sides.

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.

It works great in Compatibility now 🙂

@lander-vr lander-vr force-pushed the reflection-probe-blend-property branch 2 times, most recently from ec551b3 to f39662f Compare December 4, 2024 16:25
@lander-vr lander-vr force-pushed the reflection-probe-blend-property branch from f39662f to 0501018 Compare December 5, 2024 22:38
@lander-vr
Copy link
Contributor Author

Rebased to resolve conflict. :)

@Repiteo Repiteo merged commit 7c015a7 into godotengine:master Dec 12, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 12, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

6 participants