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 priority property to ReflectionProbe #8167

Closed
jcostello opened this issue Oct 17, 2023 · 7 comments · Fixed by godotengine/godot#100241
Closed

Add a priority property to ReflectionProbe #8167

jcostello opened this issue Oct 17, 2023 · 7 comments · Fixed by godotengine/godot#100241

Comments

@jcostello
Copy link

Describe the project you are working on

3D lighting

Describe the problem or limitation you are having in your project

Its really common to have a general reflection probe for a part of a level and small reflection probes for small details or highly reflection props in order to have acuarte reflections

The problems is that Godot always blends the reflection probes together:

image

This can be mitigated once this PR is merged godotengine/godot#50196 but if only mask out the reflection from a mesh, but sometimes (specially for objects that are moving) you want a probe to have a priority over other instead of a blend.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It would be really useful to have a priority integer property in the reflection probe so when probes fully overlap it wont blend them together, instead reflecting based on the priority of the reflection probe.
With this, maybe a blending distance would be helpful for a transition but out of that distance, the priority will take the effect.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

If this enhancement will not be used often, can it be worked around with a few lines of script?

Nope

Is there a reason why this should be core and not an add-on in the asset library?

Reflection Probes are core

@Calinou Calinou changed the title Reflection Probes priority over other probes Add a priority property to ReflectionProbe Oct 17, 2023
@WickedInsignia
Copy link

Yes this is definitely needed, was just in a situation I could use it recently. Unity does this with a simple integer as you mentioned and whatever number is highest gets priority (if an object if overlapped by a reflection probe with priority 1, and another one with priority 2, then the priority 2 node will have preference.)

@jcostello
Copy link
Author

This is another example of why this is needed.

In the scene there are 2 ReflectionProbes. One general for the whole scene and one for the interior of the structure. Ideally, with priorities, the inside should only reflect the probes for the inside.

This is how is currently working
image

This is how it kind should look (I disabled the external probe, the sky is reflected by default)
image

@BastiaanOlij
Copy link

BastiaanOlij commented Feb 8, 2024

As I'm currently working on reflection probes for compatibility I might have a look at this. If it is acceptable to drive this purely on a priority/importance setting I think it's fairly easy to implement and still keep the old behaviour.

For compatibility/mobile renderer it will simply be a case of adding a priority check to pair_reflection_probe_instances, basically we get something like this:

paired_reflection_probe.clear();
int importance = 0;

for (int probe = 0; probe < p_reflection_probe_instance_count; probe++) {
  int probe_importance = LightStorage::get_singleton()->reflection_probe_instance_importance(p_reflection_probe_instances[probe]);
  if (probe_importance > importance) {
    importance = probe_importance;
    paired_reflection_probe.clear(); // reset
    paired_reflection_probe.push_back(p_reflection_probe_instances[probe]);
  } else if (probe_importance == importance {
    paired_reflection_probe.push_back(p_reflection_probe_instances[probe]);
  }
}

With the clustered renderer it will be a little more tricky because we can't do it at a tile level, we'll have to be a bit creative in the scene shader. But I think I have an idea for that as well.

Might not get to this right away but if this is acceptable, willing to do a PR for this.

edit thinking of this more, this might not be solvable in pair_reflection_probe_instances, it may need to be implemented in the fragment shader for compatibility/mobile as well. Will need to experiment a little to see if there is a performant way to do this.

@rockoko
Copy link

rockoko commented Mar 27, 2024

Coming from working professionally in Unreal Engine for several years, I sort of thought a smarter blending behaviour being just how it is supposed be. Setting prio would be a workable solution for local reflection probes on highly reflective objects, but I think I'd prefer the way Unreal handles it, which is simply prioritising the smaller probe inside the bigger and then blending out the smaller one earlier as culling is based on screen size.

@jcostello
Copy link
Author

Coming from working professionally in Unreal Engine for several years, I sort of thought a smarter blending behaviour being just how it is supposed be. Setting prio would be a workable solution for local reflection probes on highly reflective objects, but I think I'd prefer the way Unreal handles it, which is simply prioritising the smaller probe inside the bigger and then blending out the smaller one earlier as culling is based on screen size.

This case is more flexible because it let you choose same priority to blend the reflections

@unfa
Copy link

unfa commented Nov 25, 2024

Heh. This would fix my problem with trying to implement a 3D skybox.
I want to provide a reflection probe to make the 3D skybox reflect properly, but now if I do that, the larger reflection probe overrides all small ones inside making interior reflections broken.

I suppose automatic prioritizng the smaller reflection probe inside a bigger one is a sensible default. Optionally a manual prority could be useful for achieving special effects.

@lander-vr
Copy link

This case is more flexible because it let you choose same priority to blend the reflections

I'm thinking this could probably just be a checkbox which when checked matches the priority with the parent probe. I can't really imagine a situation where you'd want a smaller probe to have a lower priority than the larger one it's within, the reflections would just get overwritten.

The ease of use of having automatic priorities is huge, which I think this snippet demonstrates quite nicely.

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