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

Reflection probes don't update their size when upgrading a project to 4.4 #102848

Open
FlooferLand opened this issue Feb 14, 2025 · 7 comments
Open

Comments

@FlooferLand
Copy link
Contributor

FlooferLand commented Feb 14, 2025

Tested versions

  • Reproducible in v4.4.beta3.official [06acfccf8]

System information

Godot v4.4.beta3 - Windows 11 (build 22631) - Multi-window, 1 monitor - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1660 SUPER (NVIDIA; 32.0.15.7216) - AMD Ryzen 5 3600 6-Core Processor (12 threads)

Issue description

When upgrading a project to 4.4, reflection probes remain the same size/shape.
This is a problem specifically for interior scenes, as users size probes to perfectly fit the interior of their rooms, and making a probe larger than the room worsens reflection quality.

Converting to 4.4 causes those walls to fall into the outside "blend" area of the probe, breaking reflections for walls
(see the gif below, the probe is sized to firmly fit the room)
Image

I believe #99958 to be the cause, but I'm not fully sure.

- Solutions -
The best solution to this in my opinion would be adjusting reflection probes to make the blend area extend outside of the reflection probe's shape, instead of insetting based on the size of the reflection probe. This would also be more user-friendly in my opinion, as I wouldn't want to manually resize my reflection probes to fit my room every time I want to adjust the blend distance.
The easiest solution would be setting blend_distance to 0 during conversion, or calculating and resizing every reflection probe based on the default blend_distance of 1

Steps to reproduce

  1. Create a new project in 4.3
  2. Make a room out of CSG shapes, add reflective materials to them
  3. Add a light, preferably a spotlight that does not touch the walls, as it makes it easier to see if reflections are working
  4. Add a reflection probe and adjust its size to fit inside of the CSG walls
  5. Convert the project to 4.4, and you will see the reflections are now broke and require manually resizing the reflection probe's green interior box to fit inside the room

Minimal reproduction project (MRP)

mrp-4.3.zip
mrp-converted-4.4.zip (direct result of
converting mrp-4.3.zip to 4.4)

@akien-mga
Copy link
Member

CC @lander-vr @Calinou @clayjohn

@github-project-automation github-project-automation bot moved this to For team assessment in Rendering Issue Triage Feb 14, 2025
@akien-mga akien-mga moved this from Unassessed to Bad in 4.x Release Blockers Feb 14, 2025
@Calinou
Copy link
Member

Calinou commented Feb 14, 2025

The best solution to this in my opinion would be adjusting reflection probes to make the blend area extend outside of the reflection probe's shape, instead of insetting based on the size of the reflection probe. This would also be more user-friendly in my opinion, as I wouldn't want to manually resize my reflection probes to fit my room every time I want to adjust the blend distance.

This also sounds like the best solution to me, but it's likely nontrivial to implement given blend_distance will affect the probe's actual size. The editor gizmo code for ReflectionProbe would have to be changed so the handles are not set according to size, but size + blend_distance.

@lander-vr
Copy link
Contributor

lander-vr commented Feb 14, 2025

I don't think there is a way we can accommodate the reflection probe changes so all refprobes automatically are set up correctly in 4.4. The probes were so flawed before that there's pretty much a guarantee that they are not set up correctly in any project, and I unfortunately think having users go through their refprobes and setting them up manually is unavoidable due to how unique each situation can be.

The issue showcased in the mrp is a bit exacerbated due to the interior property being toggled. Your setup that's "broken" in 4.4 would also not be a good setup in 4.3 if you weren't using the interior property, because all walls sit at the border of the blend influence of the reflection probe, and barely receive any contribution from the probe because of this. The only reason that setup works in this case is because the interior property did not blend with the surroundings at all, and just created a hard cutoff at the outside bounds.

In the following image (4.3) I have given the probe a solid color, disabled interior, and extended the probe vertically so we get a sort of slice-through view of their contribution in this space:
Image
Image
You can see that the walls barely receive any reflection contribution area. But when we toggle the interior property:
Image
... all blending disappears.

Here's the inside with interior toggled and not toggled:
Image

The scenario in the mrp would be easily fixed just by setting the blend-distance to 0, but doing that as a default doesn't make sense for so many other scenarios.

The proposed solution of having the blend_distance move outwards instead of inwards might make sense in this isolated situation, where there is no "other side of the wall" to worry about, and where you are not dealing with a blend distance at all due to the lack of blending with the interior bool, but in most situations the proposed solution doesn't make sense either.
E.g. think of a room with an exterior wall, it would now have reflection bleeding on the outside wall. A house with multiple floors would now have reflections bleeding through all the floors, etc. I also think in terms of workflow it makes much more sense to determine the outer bounds first, then the inner bounds. Additionally, given how broken some of the behavior of refprobes were, I just don't think it makes sense to make a change like this for the transition to 4.4, it would ultimately lead to users having to deal with a strange workflow always, as opposed to biting the bullet just once.

It's unfortunate, but I think the changes introduced with both the blend distance and priority blending PRs are such extensive behavioral changes of reflection probes that I don't think there's any way to go about this besides for users to go through their reflection probes and set them up correctly, now that they're able to do so.

@lander-vr
Copy link
Contributor

Just pinging here to check whether or not we can consider this closed. I don't think there's really anything proactive we can do here to accommodate this transition, besides maybe mentioning this on the 4.4 release post?

I am still meaning to update the refprobe documentation to reflect 🙃 the new behaviors and explain intended workflows around it. Hopefully I can find some time for that this weekend.

@Calinou
Copy link
Member

Calinou commented Feb 20, 2025

I agree we should just document this change in the release notes and in the Upgrading to Godot 4.4 manual page.

@FlooferLand
Copy link
Contributor Author

FlooferLand commented Feb 21, 2025

I would still recommend changing the blend distance of reflection probes to 0 for projects upgraded from 4.3 on top of documenting the change, as this reflects the original 4.3 behaviour and the user can always manually reset their blend distances back to 1.
If the user is confused why the probes aren't blending, they will likely realize the blend distance is set to 0 and reset it. I didn't realize the reason my walls weren't reflecting was tied to the walls being outside the blend distance, though that's likely to be a gizmo readability issue or it might just be me.

A notice in the 'Upgrading to 4.4' page would be good enough, this issue should probably close whenever the documentation change is implemented?

Unrelated, but the project upgrader should probably link to the corresponding version's upgrade manual page after the user upgrades their project. I didn't realize there were pages for manual intervention during upgrades up until now

@lander-vr
Copy link
Contributor

as this reflects the original 4.3 behaviour

Only in cases when the interior property is toggled, but since setting it to 0 isn't really a recommended setting and doesn't make sense in the vast majority of situations, I'm not convinced of setting the blend distance to 0. Even if we do it just for probes with the interior bool toggled.

I don't think there's really any going around it, regardless what we set it to, pretty much everyone will have to make adjustments or move/replace their probes, if not for tweaking the blend distance parameter then as a result of the new sorting behavior.

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

No branches or pull requests

7 participants