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 a Fade Start property to ReflectionProbe #61416

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 25, 2022

The default value of 0.5 makes reflections from ReflectionProbes more visible. This significantly reduces the notorious "sky is visible through solid objects in reflections" issue that people tend to notice when setting up reflection probes in Godot. If you set it to 1.0 in an indoor scene, the issue should be entirely prevented now, provided the probe's origin is not within solid one-sided geometry.

Please test this on more complex scenes to check if the proposed default value is actually a good idea. It may be a bit too aggressive; perhaps 0.2 would be a better fit.

To get the old behavior back, set Fade Start to 0.0.

See godotengine/godot-proposals#3013.

Testing project: test_reflection_probes_mirage_palace.zip

Preview

Before (Fade Start 0.0) After (Fade Start 0.5, default)
Screenshot_20230925_192235 webp Screenshot_20230925_192242 webp

Fade Start 1.0 is meant to be used when you have a single probe cover an entire room, so it's not the best fit for this situation, but here it is anyway for comparison purposes:

After (Fade Start 0.85) After (Fade Start 1.0)
Screenshot_20230925_192250 webp Screenshot_20230925_192254 webp

TODO

  • Implement in Compatibility.

@Calinou Calinou requested review from a team as code owners May 25, 2022 18:38
@Calinou Calinou added this to the 4.0 milestone May 25, 2022
@Calinou Calinou force-pushed the reflectionprobe-add-fade-start-2 branch from a4af986 to 435663f Compare May 25, 2022 18:39
@Calinou Calinou changed the title Add a Fade Start property to ReflectionProbe, improve blending formula Add a Fade Start property to ReflectionProbe May 25, 2022
@Calinou Calinou force-pushed the reflectionprobe-add-fade-start-2 branch from 435663f to cdd4527 Compare May 25, 2022 21:35
@jcostello
Copy link
Contributor

jcostello commented Jun 16, 2022

Works fine but with Fade Start at 0 between a inside and outside probe you get that fine line

Edit: I ment 1.0 for Fade Start

image

@Calinou
Copy link
Member Author

Calinou commented Jun 16, 2022

Works fine but with Fade Start at 0 between a inside and outside probe you get that fine line

Fade Start = 0.0 is the existing behavior in master. Do you get this issue without this PR?

@jcostello
Copy link
Contributor

@Calinou sorry, I ment 1.0

@Calinou
Copy link
Member Author

Calinou commented Jun 17, 2022

@Calinou sorry, I ment 1.0

In that case, I'm not sure how this can be prevented due to floating-point precision issues. You can make ReflectionProbes slightly overlap each other to work around this.

@jcostello
Copy link
Contributor

jcostello commented Jun 17, 2022

They are overlap already. That happen at the end of the Reflection Probes set as inside: false

Edit: One way of solving this is to add a parameter fade away the sky from the edge of the reflection probes and other parameter to fade away the probe into another probe. This will fix transitions like this also

image

@jcostello
Copy link
Contributor

They are overlap already. That happen at the end of the Reflection Probes set as inside: false

Edit: One way of solving this is to add a parameter fade away the sky from the edge of the reflection probes and other parameter to fade away the probe into another probe. This will fix transitions like this also

image

That way you can completely fade away the sky reflection in the edge but smootly fade away the probe with other probes and avoid this issue

image

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'm not sure that this addresses the use-case in godotengine/godot-proposals#3013

This change may be helpful as it allows users to control where the blend starts and thus may help with some edge cases, but ultimately, the problem will still exist. Take for example, the example in the OP. With this change, the user could tweak the blend start so that very little light leaked into the room, by setting the fade start at roughly the room extents. The issue is this just pushes the blend portion outside of the room, so you will end up with occlusion artifacts outside of the room. What is needed is for us to save a depth map with the reflection probe and then use the depth map to blend.

@Calinou
Copy link
Member Author

Calinou commented Jan 21, 2023

One more situation where this popped up: https://www.reddit.com/r/godot/comments/10gux8y/why_is_the_skybox_overlaying_the_reflection/

In this particular example (mostly outdoor scene), increasing Fade Start would do the trick well.

What is needed is for us to save a depth map with the reflection probe and then use the depth map to blend.

Is this cheap enough to be feasible on the Forward Mobile backend, let alone Compatibility? Would this be something you need to enable on every ReflectionProbe (for performance reasons), or would it always be done?

@atirut-w
Copy link
Contributor

What is needed is for us to save a depth map with the reflection probe and then use the depth map to blend.

Isn't that literally the same as adding lights with shadows to the scene? That can't be good for performance if it's enabled by default despite it fixing issues. I think being able to customize fade start is many times better.

@Calinou Calinou force-pushed the reflectionprobe-add-fade-start-2 branch from be108b6 to 69e800c Compare September 25, 2023 17:20
@Calinou
Copy link
Member Author

Calinou commented Sep 25, 2023

Rebased and tested again in both Forward+ and Mobile, it works as expected. (Compatibility doesn't support reflection probes yet.)

@jcostello I've fixed the issue you mentioned by decreasing the MAX() threshold from 0.001 to 0.00001 in the inv_fade_start setter on the C++ side.

@jcostello
Copy link
Contributor

@Calinou nice, I'm testing this again

@WickedInsignia
Copy link

Artifacts have expired and I'd like to test this since my original proposal is being mentioned quite a lot here. I agree with ClayJohn, I'm not sure this actually resolves the proposal. Is there a build with this available somewhere?

@atirut-w
Copy link
Contributor

atirut-w commented Oct 19, 2023

Is there a build with this available somewhere?

If you dont mind running a build someone else compiled, I can make one.

EDIT: Hold on, can the automatic builds be re-run?

@WickedInsignia
Copy link

Is there a build with this available somewhere?

If you dont mind running a build someone else compiled, I can make one.

EDIT: Hold on, can the automatic builds be re-run?

When testing features I’d prefer to use an automatic or official build for security reasons. Thank you for the offer though!

@Calinou Calinou force-pushed the reflectionprobe-add-fade-start-2 branch from c4c72d0 to 774ddc2 Compare October 19, 2023 16:02
@Calinou
Copy link
Member Author

Calinou commented Oct 19, 2023

I've pushed a rebase with no changes to create a new build (check back in 1 hour).

I agree with ClayJohn, I'm not sure this actually resolves the proposal.

I don't intend this PR to resolve the linked proposal, which is why it doesn't say "closes" before the PR link. However, I believe this feature is still needed for artistic control (and it can help somewhat if you encounter the sky occlusion situation).

@jcostello
Copy link
Contributor

I've pushed a rebase with no changes to create a new build (check back in 1 hour).

I agree with ClayJohn, I'm not sure this actually resolves the proposal.

I don't intend this PR to resolve the linked proposal, which is why it doesn't say "closes" before the PR link. However, I believe this feature is still needed for artistic control (and it can help somewhat if you encounter the sky occlusion situation).

I agree. I would have it merge since it gives control over the reflections.

In order to fix the underline issue, how complicated is to capture another sky reflection but with the geometry oclussion instead of using the whole sky box?

@Calinou
Copy link
Member Author

Calinou commented Oct 19, 2023

In order to fix the underline issue, how complicated is to capture another sky reflection but with the geometry oclussion instead of using the whole sky box?

As mentioned in clayjohn's comment, you need to store a depth cubemap along the color cubemap (or perhaps store it in the cubemap's alpha channel so that no separate texture is needed). This will require slightly more time to update the cubemap but it should allow for properly occluding the sky.

This may also allow for improving parallax-corrected cubemaps with something similar to parallax occlusion mapping, as seen in God of War Ragnarok's fidelity mode.

@jcostello
Copy link
Contributor

This will require slightly more time to update the cubemap but it should allow for properly occluding the sky.

I don't mind that requires more time. In the future, cubmaps should be baked in editor unless they are move or updated always

@atirut-w
Copy link
Contributor

As mentioned in clayjohn's comment, you need to store a depth cubemap along the color cubemap (or perhaps store it in the cubemap's alpha channel so that no separate texture is needed).

Completely disabling blending when fade start is 1.0 might also work, I think

@WickedInsignia
Copy link

WickedInsignia commented Oct 20, 2023

In testing this definitely doesn't solve that proposal (that's not really news though). Multiple exterior probes in an interior with fade at 1 creates too many artifacts at the boundaries, and setting these probes to Interior nullifies exterior reflections.
This is a great setting to have though, and alongside probe prioritization + depth cubemaps + custom per-probe resolution would improve reflection probes considerably.

Exterior Reflection probe, fade at 0.5:
ReflectionProbe_fade05

Exterior reflection probe, fade at 1:
ReflectionProbe_fade1

Interior reflection probe. Exterior reflections are far less present:
ReflectionProbe_Interior

One exterior reflection probe for the bar, intersecting with the exterior reflection probe for the room. Both have a fade at 1. This artifact on the wood is too obvious and both probes would need to be interior instead:
ReflectionProbe_InteriorFade1

Couple important things I've noticed:

  • There seems to be some frustum culling of selected reflection probes in this PR. The borders and handles were disappearing at certain angles for me. This is very apparent in the Unreal Sun Temple, and doesn't happen in 4.2 Beta 2.
  • This property is very difficult to edit and see its effects when the reflection probe's bounds are represented by a near-opaque green border. I might need to open a proposal to change this to something less visually obscuring.

UPDATE: Proposal to change the probe bounds HERE.

@jcostello
Copy link
Contributor

jcostello commented Oct 20, 2023

Definitely probe prioritization and depth cubemaps should be a must to have acuarte results. @clayjohn can you write a propsal (or comment in the OP proposal) about depth cubemaps since you know better what should be done?

@atirut-w
Copy link
Contributor

Is this PR dead? I'm a little worried considering that this (OR the depth cubemaps solution) is pretty important to have.

@WickedInsignia
Copy link

Is this PR dead? I'm a little worried considering that this (OR the depth cubemaps solution) is pretty important to have.

Unfortunately since the Godot project does not have individual task prioritization or a comprehensive public-facing roadmap we have no idea. I have pushed for both in the past for this very reason. It doesn't matter how in-demand a feature is: either a core team member has to be interested enough to do it or an external contributor needs to come along and implement it themselves. In the case of the latter, there's a chance it could be stuck in yet-to-be-merged limbo for a while.

I also consider this an important feature and as of right now, reflection probes aren't useful for mixed exterior/interior spaces.

The default value of 0.5 makes reflections from ReflectionProbes
more visible. To get the old behavior back, set Fade Start to 0.0.
@Calinou Calinou force-pushed the reflectionprobe-add-fade-start-2 branch from 774ddc2 to d4dac0d Compare October 7, 2024 17:34
@Calinou
Copy link
Member Author

Calinou commented Oct 7, 2024

Rebased and tested again, it works as expected. I've implemented the feature in Compatibility as well, following #88056.

@atirut-w
Copy link
Contributor

atirut-w commented Oct 8, 2024

BOOYAH! This will probably get marked as off topic, but I'm so glad this is not dead, considering how essential of a feature this is. I hope this is getting cherry picked once its done?

@Calinou
Copy link
Member Author

Calinou commented Oct 10, 2024

I hope this is getting cherry picked once its done?

As per the release policy, new features are not cherry-picked to previous releases to minimize the risk of regressions.

@lander-vr
Copy link
Contributor

lander-vr commented Nov 27, 2024

Please test this on more complex scenes to check if the proposed default value is actually a good idea. It may be a bit too aggressive; perhaps 0.2 would be a better fit.

I would argue for the opposite, a higher default value makes more sense. Especially with the context of the probe priority PR (#89408).

Generally you'd want whatever is within the volume of a probe to be able to fully rely on that probe by default, and just want a bit of blending towards the probe borders to avoid a sudden obvious change. Since probe workflows very commonly involve having nested probes (for which we need probe priority), a small fade start would introduce a lot of duplicated reflections. A default factor of 0.5 means half of the area covered by the probe experiences those double reflections, which is essentially leakage from other probes, this is definitely something we'd want to minimize by default.

I tried to demonstrate this in a test project, it is not an ideal demonstration due to probe prioritization not being implemented yet, however it shows the effect that would happen on these transition areas between probes:
image
-> A single window turns into three reflections. (The way these probes are set up would obviously not be ideal even with probe prioritization, however it does the job of showcasing the issue) You can also see this effect on Suzannes head, where you can easily spot duplicates. This would happen on all transition areas. A low value of 0.2 would mean these would be very prevalent.

I also wanted to note that "band" of brighter reflections along the transition area is caused by the skylight getting blended in. This band should not be there once we have priority blending.

Something like 0.75 seems like a more appropriate default value to me: You get a bit of blending at the borders as a smooth transition into the probe, and the majority of objects covered by that probe will receive clean (i.e. not double or blended) reflections.

I do think part of the reason it might feel too aggressive is because for the ambient light from probes these higher values are in fact too aggressive as a default. I think it could be worth considering having a separate fade value for the ambient light, since ambient light is a much lower frequency effect compared to reflections. This would allow great artistic control of minimizing visible seams between nested probes (especially probes placed for detailing) where you'd still want the probe to take over reflections quickly (i.e. high-fade start), but want the ambient light influence to remain subtle and blend well with the "parent" probe.

Lastly, I think it could make sense to rename and re contextualize this property. Fade start makes sense as a property name when you look at what happens under the hood, but I think from an artists POV it would be a more intuitive experience if 0 means no blending (sharp edge), and increasing this parameter increases this blending distance from the edge of the probe. The property could be name "blending" (Or reflection blending, ambient blending)

Besides these things I've brought up: the behavior of the fade start works mostly as expected, though having non-cubic probes means there are inconsistencies with blending distances on the various sides of the probe. I could imagine this be an issue for things like alleyways, corridors, etc.
Shown here using a custom ambient color on a probe:
image
I don't think this is that big of a deal, and it shouldn't prevent this from being merged in the case that this is difficult to fix, but it is definitely something to be aware of.

@ricky-daniel13
Copy link

Lastly, I think it could make sense to rename and re contextualize this property. Fade start makes sense as a property name when you look at what happens under the hood, but I think from an artists POV it would be a more intuitive experience if 0 means no blending (sharp edge), and increasing this parameter increases this blending distance from the edge of the probe. The property could be name "blending" (Or reflection blending, ambient blending)

I believe the concept of Fade Start is used in others places in Godot? Namely the distance fade option in lights, so maybe the name is approriate.
But yeah, I'm pretty sure the expectation of someone using reflection probes is for the probe to be the main source of reflected light on objects inside its bounds. a very tight default fade start would make sense.
(Not sure if ambient light distance should be separate, I don't imagine any reason to want different values for those)

@clayjohn
Copy link
Member

@lander-vr Can you clarify whether the fade start property is needed because artists need finer control over how probes blend together, or is it needed simply to avoid the nasty artifacts that we currently get as a result of blending across the entire probe?

Based on the discussion here and in godotengine/godot-proposals#3013 it seems to me like the real problem is not that fine-tuned blending is needed, but rather we need to give artists a way to avoid having the sky and other probes blend in to the center of the current probe. Accordingly, I feel like the solution would be:

  1. reduce the current blend to a much smaller margin
  2. detect occlusion so we can improve the blending towards the sky.

The reason I ask is that this setting will reduce performance for every reflection probe and once we add it we can't take it back, even if we solve the root of the issue later.

@lander-vr
Copy link
Contributor

lander-vr commented Nov 27, 2024

I think exposing a parameter to control blending is a must. Here's a classic situation:
image
A single wall where a player can be on both sides. Considering different scenes on both sides, you want reflections of the one scene on one side, and the reflections of the other scene on the other side.

If the probe has a hard-coded blend distance, these are your only options (Blend distance portrayed by the interior boxes):
image

  • option A: Avoid reflection leakage on the other wall, with the drawback of not getting full reflection strength on the sides of the wall
  • Option B: Overlap the probes, so the wall is far enough inside the probe to be fully affected by it. This will introduce reflection leakage.

Allowing the user to set an extremely narrow blend distance resolves this situation. Which is not something you'd want by default because that'd make the edges of probes very obvious in most situations.

Now replace that wall with some foliage:
image
Here the user would want a much smoother blend distance, and possibly even some overlap, to avoid a visible seam running over the bushes

If the situation presented in that proposal was a situation where you'd also be able to go outside and see the other side of those walls, and you wouldn't be able to adjust the blend, you'd end up with either poor reflections, or still get leakage, even when detecting sky occlusion.

In the case of hypothetical spherical probes, I don't think this would be relevant and something we'd need to accommodate for.

@lander-vr
Copy link
Contributor

lander-vr commented Nov 27, 2024

I believe the concept of Fade Start is used in others places in Godot? Namely the distance fade option in lights, so maybe the name is approriate.

I think that's a different context. The probe itself as an object is not getting faded, there's also an extra parameter there called "begin". In this case there's also already a specific distance context: the bounds of the probe. I strongly believe presenting this parameter as a setting that "blends from the edges of the probe" is a much more intuitive UX for artists: number goes up -> you get more of the effect

Edit:
I just wanted to add, that even more intuitive would be if this parameter represented a distance/length. This would mean it does not scale with the size of the probe, but remains consistent, and turns it from a more abstract value into something tangible. For this to make sense though, the blending distance should probably be consistent from all sides of the probe, and not stretch.

@Calinou
Copy link
Member Author

Calinou commented Dec 3, 2024

@Calinou Calinou closed this Dec 3, 2024
@Calinou Calinou removed this from the 4.x milestone Dec 3, 2024
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.

9 participants