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

Vulkan: Need to split cull mask for reflection probes #50195

Closed
Tracked by #66628
BastiaanOlij opened this issue Jul 6, 2021 · 11 comments
Closed
Tracked by #66628

Vulkan: Need to split cull mask for reflection probes #50195

BastiaanOlij opened this issue Jul 6, 2021 · 11 comments

Comments

@BastiaanOlij
Copy link
Contributor

Godot version

4.0 dev

System information

Any

Issue description

The cullmask setting on reflection probes has a dual function.

It will determine which layers we check for objects to be rendered into the reflection probe itself.
And it determines onto which objects the reflection probe is applied.

So if I want to prevent an object from reflecting onto itself I can't. If I remove it's layer from the reflection probe it is no longer rendered into the reflection probe, which is what I want, but it no longer uses the reflection probe for its reflections, which is what I don't want :)

Both behaviors make sense on their own but they should not be combined. I believe we should add a second cull mask to reflection probes so we can handle this individually.

Steps to reproduce

Use the material tester demo, this currently has two reflection probes to work around the problem.

Remove one of the reflection probes and move the other to be inside of the mirror ball:
image

Note how the reflection looks really weird now because we're seeing the interior off the mirror ball reflected onto itself.
Note also that I have placed the mirror ball into layer #2

I suggest we keep the current cull mask working as it currently does, culling onto which the reflection probe is applied:
image

Then I suggest we add a second cull mask that determines what layers are rendered into the reflection probe, I suggest calling this reflection_mask:
image

Minimal reproduction project

godotengine/godot-demo-projects#616

@unfa
Copy link

unfa commented Feb 6, 2023

I am dealing with this in my project and it makes reflection probes quite limited.
I want to have dynamic objects only receive reflections from the probe, not contribute to them, but it's not possible to do.
The documentation however makes me think that wasn't intended behavior.

Let me link the issue I've written down in my project's repo fro this:
https://codeberg.org/Liblast/Liblast/issues/423#issue-242830

@WrobotGames
Copy link

WrobotGames commented Feb 13, 2023

quite limited.

This 'bug' also makes reflection probes unusable for any type of vehicle, like cars, where the probe needs to be inside the car for somewhat accurate reflections.

@clayjohn clayjohn modified the milestones: 4.0, 4.x Feb 23, 2023
@EntroPiGames
Copy link

I also recently ran into this issue when I was trying to add real-time reflections to a vehicle.

For now, I worked around this by placing several box projected reflection probes around the scene, but that approach also has issues because there's no way to control the rendering / baking order of the probes when the scene first loads and I haven't found a way to check if a reflection probe is already rendered.

@Calinou
Copy link
Member

Calinou commented May 3, 2023

but that approach also has issues because there's no way to control the rendering / baking order of the probes when the scene first loads

Godot should be modified to render reflection probes whose origin is closest to the camera first, especially when not all of them are rendered yet.

I'm not sure if adding a signal when a ReflectionProbe is done updating is possible or advised though, given the RenderingServer threading intricacies. We want to avoid CPU-GPU synchronization as much as possible since that eliminates parallelism, which leads to performance issues.

@clayjohn Do you think this would be possible?

@clayjohn
Copy link
Member

clayjohn commented May 3, 2023

I think it should be possible to sort ReflectionProbes before queuing them up for rendering. I'm not sure if that would be enough to solve the issue though

@EntroPiGames
Copy link

Rendering reflection probes closest to the camera first would be an improvement, but still not ideal. I'd prefer an option to bake all reflection probes in the first frame after the scene loads.

I read a suggestion in another issues to use a fade in effect while the reflection probes render (and other GI stuff converges) in the background, but without being able to control or check when reflection probes render (and the time it takes varying between machines) that sounds like a very hacky workaround.

@unfa
Copy link

unfa commented May 4, 2023

Right now in my game I have to do an extremely hacky thing where I show a "loading screen" and wait an arbitrary amount of frames for each ReflectionProbe in the game level hoping that they will all fully bake by that time. They mostly do. Unless they still don't, I start the game, spawn my characters and the last probe bakes while that is going on, producing a sector of blinding blue light due to character's spawning VFX. I don't even know if it's going to perform similarly on different systems. It's a mess.

GNSS-Stylist added a commit to GNSS-Stylist/LatoDemo that referenced this issue Aug 30, 2023
To allow things to reflect from the rig camera (mostly the lens, but also elsewhere since it's somewhat metallic).

Tried to play with layers, but failed miserably, partly due to this: godotengine/godot#50195
@IPlayKindred
Copy link

any updates on this? I am running into this problem while using reflection probes and I cant use them for vehicles

@WrobotGames
Copy link

WrobotGames commented Feb 9, 2024

This issue is fixed in 4.3 dev 3!
image
Screenshot of the inspector with the Cull mask and the reflection mask properties visible.

(By #86073)

@akien-mga
Copy link
Member

Thanks, closing as fixed by #86073.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 9, 2024
@unfa
Copy link

unfa commented Apr 5, 2024

I can't believe it! It's finally fixed!! THANK YOU!

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

Successfully merging a pull request may close this issue.

8 participants