-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
Add spherical harmonics sky #108127
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
base: master
Are you sure you want to change the base?
Add spherical harmonics sky #108127
Conversation
e265f16
to
44e90e4
Compare
Thanks! However, when trying this on Meta Quest 3, the colors come out much darker with this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, ambient lighting appears to be broken on my PC in Forward+ and Mobile when texture array reflections are disabled. It looks correct in master
, or when texture array reflections are enabled.
Testing project: test_pr_108127.zip
This seems to reflect @dsnopek's above testing where ambient lighting also appears broken (it's effectively pure black).
PC specifications
- CPU: AMD Ryzen 9 9950X3D
- GPU: NVIDIA GeForce RTX 5090
- RAM: 64 GB (2×32 GB DDR5-6000 CL30)
- SSD: Solidigm P44 Pro 2 TB
- OS: Linux (Fedora 42)
The link is broken. Also does the problem get solved if you go to
|
It makes no difference in my testing on the Meta Quest - everything is still overly dark. |
It works on my end, can you try this instead? https://0x0.st/8UUP.zip |
Thanks! The link now works. Maybe it was a temporary error. I can repro the problem now. The SH is not building if set the Ambient to "Background". |
44e90e4
to
c89f5cd
Compare
Pushed a fix! Thanks! This: if (ambient_source == RS::ENV_AMBIENT_SOURCE_SKY) {
sky.copy_spherical_harmonics_to_scene_data(p_render_data->environment, scene_state.uniform_buffers[p_index]);
} Should've been: if (ambient_source == RS::ENV_AMBIENT_SOURCE_SKY || ambient_source == RS::ENV_AMBIENT_SOURCE_BG) {
sky.copy_spherical_harmonics_to_scene_data(p_render_data->environment, scene_state.uniform_buffers[p_index]);
} @dsnopek could you try again? Sorry & Thanks! |
Sure! It's no longer overly dark, but it's a bit too green:
UPDATE: This is really weird! I've run my test project about 4 times with this PR, and running it for about ~1min on the 4th time, suddenly the greenness went away and everything looked normal. It didn't happen any other time, despite definitely spending at least 1 minute testing it on other occasions. I just restarted the app again, and it went back to being green |
I assume the green-ness happens only on device and not in your computer? If so, I very strongly suspect the green-ness will go away if you define
Edit: Adding some context. The SH are generated via a Compute Shader and a parallel sum reduction via Shared Memory. Since Compute Shaders, compute inline barriers and the use of Shared Memory have a history of uncovering driver bugs; I wouldn't be surprised the problem is a driver bug.
|
I've only been testing on the Meta Quest, and not on my computer. I just tried on my computer for the first time (unchecking UPDATE: Actually, I did end up getting the greenness for a couple seconds in the editor on desktop! Then it went back to normal. I'm not sure how to trigger it. (This was compiled with
I tried this and, unfortunately, it didn't help on the Meta Quest - it's still very green :-/ |
Things that come to mind:
Do you have the project so I can test it? I see that you're using godot-4-3d-third-person-controller but it appears to be modified (since the original demo uses Sky instead of Background for ambient setting). Are there any validation errors when run with |
I'm using this branch on my fork: https://github.com/dsnopek/gdquest-tps-demo/tree/standalone-vr-slush
There are some validation errors that I don't usually see: Validation messages
There's some I'm used to seeing, and I think I've filtered those out correctly |
Ok that would definitely explain the problem. I don't know why I'm not seeing them, but I do remember seeing that Godot had different RGB10A2 vs RGBA16F paths for PC and Mobile. |
To clarify, it's not platform-specific but renderer-specific. Forward+ always uses RGBA16F, while the Mobile renderer always uses RGB10A2 (even on desktop platforms). |
Thanks! That saves me time! |
c89f5cd
to
25ceaf2
Compare
@dsnopek OK fixed! I was able to repro the bug on an Adreno device. The problem was simply I also tweaked the math to prevent divisions by 0 from happening. Regarding your validation errors: I didn't look into it; but it became apparent after reading them that the problem was caused by already existing code; so the problem could be on While looking into this bug, I stumbled into #108317 which can cause problems when generating the cubemap and the SH. |
Do you have a link for the 3rd (starting from the top) HDRI? I want to take a look at that one since it looks the flattest and dull when it appears it shouldn't. The 2nd HDRI looks like a greyish environment with a strong "diagonal" light on the bottom lower corner. If that is the case, that is the worst case for SH, since L1 order is bad at high frequency data (i.e. small & sudden changes in brightness or colour). Regarding the dot: I can see it too and I am now looking into it. |
User Geomerics' SH version Optimize evaluate_sh_l1_geomerics() function SH required denormalization User Fibonacci Sphere sampling for more uniform sampling
25ceaf2
to
5709832
Compare
OK pushed a fix for the yellow (sometimes purple) dot. Sometimes the length of "dir" was above 1 I'll await for that HDRI so I can take a deeper look (I mean this one in case there's confussion): Edit: The reason I ask is because I cannot reproduce that "dull" look you mention of. I am only comparing Array vs SH version (i.e. not against ground truth) since SH is using the same input as Array. It looks slightly different, yes; but not as dull and with lack of contrast like your results. I want to use the same HDRIs to understand what's going on and whether it's a math problem, a limitation of the technique, or a software bug (i.e. your machine for some reason is incorrectly calculating the SH). |
Here are all the HDRIs I used. And in case it's useful to you in any way, here's my specs: |
@@ -418,6 +425,8 @@ void SkyRD::ReflectionData::create_reflection_fast_filter(bool p_use_arrays) { | |||
} | |||
} | |||
RD::get_singleton()->draw_command_end_label(); // Filter radiance | |||
|
|||
copy_effects->calculate_sh_from_cubemap(downsampled_radiance_cubemap, sh_coeff_buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the downsampled_radiance_cubemap
might partly explain why the SH version is missing highlights. It uses either a half res cubemap (when using the "high quality" path) or a 64x64 cubemap (when using the "fast filter" path). In either case its a lower res cubemap that is already blurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit confused on where I should grab the texture from, I thought I got it from the right place (resolution is 128x128 IIRC) but it's definitely possible that it's wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch that perhaps the problem is simply the wrong input.
Eevee looks like it is using L2 spherical harmonics. Here is a reference that compares L1 to L2. https://therealmjp.github.io/images/sss/sh_dirlight_comparison.png Notice how the best you get with L1 is basically a smooth gradient, while with L2 you can get a clear band. L2 is much better at approximating directional lighting as a result. However, the question we have to ask is whether we should be using environment maps to encode directional lighting. Typically in games you remove the directional lighting from an HDRI before using it since it is better approximated with a DIrectionalLight rather than encoding into the IBL (https://blog.selfshadow.com/publications/s2016-shading-course/unity/s2016_pbs_unity_hdri_notes.pdf pg 71 and https://seblagarde.wordpress.com/wp-content/uploads/2015/07/course_notes_moving_frostbite_to_pbr_v32.pdf pg 59). In Godot, this has been a long standing issue because the HDRIs you download online from places like Polyhaven do not remove the directional lighting (the bright highlight from the sun), and users expect to be able to drop in the HDRI directly and have it look as good as in Blender. Which, isn't really feasible to do in real time. Also note, Filament appears to use L2 when encoding to SH (https://google.github.io/filament/Filament.html#lighting/imagebasedlights/distantlightprobes), |
I haven't looked at the provided HDRIs yet, but one of the reasons I want to take a look is that I can hack the 1st order multiplier and you get much higher contrast and directionality. While it's not PBR-correct, artistically it looks very pleasant and satisfying to the eye. Thus I want to explore that a bit to see if it's worth it. |
the 9 coefficients for L2 SH is still far less than even the minimum 24 pixels of the 2x2 per face cubemaps that could be used currently. happy to test performance with both L1 and L2 for my VR app, if performance is the concern |
Its not the storage size that we are worried about for L2 (since we are storing the cubemap anyway) Its the cost of reconstruction. L2 requires more than 2x the number of instructions to reconstruct compared to L1. Therefore it is significantly slower than L1. |
To explain it in layman terms, Spherical Harmonics is much like lossy-compressing a picture in JPG. It uses math to encode color (in fact JPG is very similar because both SH and JPG work & compress in the frequency domain). However that means reconstructing with higher fidelity means evaluating more math operations, thus at L2 we need a ton of muls and additions, despite having "just" 9 coefficients per component. A cubemap may only store 24 pixels. (6x2x2), but it is very fast because at any time we only need to lookup 1 pixel and 3 more of its neighbours (to perform bilinear interpolation). This is cheap because the cubemap stays decompressed at all times. But just like 12kb of JPG can easily store 200kb of BMP data or more, "Just" 4 coefficients of SH L1 compress a lot more than 24 pixels. |
i forgot that the ALU costs for higher order SH are what they are. I'm almost always drawcall or texture sample bottlenecked for webXR so it's not something I think of as much. Maybe if SH are planned to entirely replace cubemaps for ambient lighting, there could be an option to pick between L1 and L2, since there may be cases where the sky lighting has enough directional information to make L2 worth it |
I was experimenting with the moon lab HDRI from @lander-vr and noticed that there may be additional issues with the HDRIs that we just aren't handling right now. I tried enabling process/hdr_clamp_exposure = false process/hdr_clamp_exposure = true I suspect this is due to an overflow at some point in the import / rendering process process/hdr_clamp_exposure = false process/hdr_clamp_exposure = true Note: All these screenshots are taken with Beta 1 edit: Okay I confirmed, the darkening is from pixel overflow. There is a single pixel that maps to INF when the sky texture is reduced to RGBA16 |
OK I took a look at the HDRi provided. At a glance it looks like simply a limitation of SH L1. However directionality can be boosted (it's no longer PBR-correct though). Boosting directionality can brighten or darken the lighting. Normal SH2x Boosted SHAdding a slider to boost directionality can give artists more control. |
An important thing to consider is that ambient light sources themselves often do have lots of directionality, as they usually don't cover a hemisphere (let alone cover a hemisphere uniformly), and I'm afraid L1 SH simply wont be able to deal with the slightest contrast, even row 2 and 5 in my comparison table seem like reasonable amounts of directionality that I feel like should be able to be conveyed. That isn't really something you can tackle using directional lights. I imagine L1 wouldn't be able to really deal with even a sunset sky, where you'd want to differentiate between the warmer ambience from the sunset side, reflected light from the ground, and the dark side on the "night" side of the sky. Example from a sunset HDRI:
I'd argue exploring L2 SH could still be worth it, and keeping L1 SH as an option as @michaelharmonart suggested, similar to the current cubemap arrays vs a single cubemap option, because while we are getting an extremely subtle gradient with L1 SH, practically speaking it's pretty close to just a solid color ambience. |
Note
We were pressed for time so testing is appreciated (both performance, quality, and stability).
For ambient lighting Godot supports using cubemap arrays vs a single cubemap (controlled by
rendering/reflections/sky_reflections/texture_array_reflections
). The former gives a lot more quality*, but is expensive. The latter is simply faster and more compatible.However the "single cubemap" version has two problems:
This PR replaces the use of the "single cubemap" version for L1 Spherical Harmonics in hopes of achieving:
Evaluating performance in all devices is complex. Bandwidth-limited GPUs are going to prefer the SH version, while ALU-limited GPUs will prefer the cubemap version. Furthermore it is to be expected that the cubemap version could win in very high resolutions (e.g. 4k rendering) due to texture cache effects; specially on Desktop GPUs that are able to hide the texture fetch latency.
With @clayjohn we are considering making the SH version the only version available for everything (i.e. get rid of the cubemap array too!) to:
Cubemaps arrays are not entirely going away though, because they are still used for specular reflections. This only affects "sky" ambient lighting.
Note
To test this feature you must set
rendering/reflections/sky_reflections/texture_array_reflections
to false(*) I'm not exactly sure about quality, because comparing the cubemap array, it's very apparent it's significantly brighter than it should be.