-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add bevy_light::SunDisk #20434
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 bevy_light::SunDisk #20434
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
1de0f14
to
6c6bb45
Compare
I'm very interested in this! Seems extremely useful artistically and very sensible to expose. |
Hey everyone, this is a very neat! Thank you for putting together this PR. I do have a pending change to include the angular radius in directional light, as well as a new SunLight component. I could tac on this change to the SunLight component itself rather than the atmosphere settings. So there might be some conflicts. I will leave a more detailed response with my specific suggestions for the user facing API later as I have thought extensively about this problem based on all the feedback received so far. Also regarding integrating the atmosphere with probes, another problem area that I have addressed in my fork of Bevy, still needs to be reviewed and merged to main. |
#19037 is the relevant PR btw. Currently closed for rebasing and splitting |
Hi @mate-h, thanks for the extra context! I had a quick look at your PR and I’m a bit unsure about the ergonomics of the approach. If I’ve got it right, you’re proposing a pub struct SunLight {
pub angular_size: f32,
pub intensity: f32,
} My initial aim was to let GI-probe cameras hide the sun disk while keeping it visible for the main camera. That fit well with If sun-disk visibility is moved onto the |
What is your current approach with GI probe cameras? I am also proposing to add another component called atmosphere environment map light that does that part. It is meant to upsample the sky view LUT (or do a raymarch) into a target cubemap texture, in a separated shader without the sun disk. Then it is plugged into the (recently merged) environment map filtering pipeline to get cubemap mip chain at different roughness levels. I could still see the need for a separate decoupled intensity setting for the sun disk itself. But it would only affect the composited sky in the main sky rendering pass, not the environment light. There the disk is deliberately excluded, and instead we sample the transmittance LUT in the pbr pipeline to tint the directional lights orange at sunset. In its present state Bevys atmosphere is only a composited effect but the changes I am proposing would fully integrate it with the PBR pipeline and environment reflections. Feel free to reach out on Discord (user mate_h) to discuss the approach and the details. |
There is a newer , working version mate-h#10 But I need to work on splitting it up a bit more. |
Angular sun disk size for directional lights is also something I'm interested in. The code for it is already in Solari, it's just not exposed to users :) |
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.
Approved once the wording change I proposed is merged
I like the idea of having a stylistic adjustment for the sun-disc intensity, but I think I'd prefer something like @mate-h's proposed Also (and this is more editorial) maybe the intensity override should be an |
e7f8863
to
0a25380
Compare
Isn't the itensity supposed to affect the SunDisk, and not the light itself? |
Fyi you can probably delete SUN_ANGULAR_DIAMETER_RADIANS now. Not sure why CI didn't warn that it's now unused 🤔. |
You are right, but the same argument is also applicable to But As result, in the current implementation, Solari relies on I don’t have a better proposal, tbh, just highlighting the little mess we're creating here. I’d prefer to rely on your and @mate-h decision, anyway, since it highly depends on vision of final workflow of solary + atmosphere. |
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.
Great work on the new approach! looks solid with no blockers from my side.
- Shadows vs sun disk size: Keep these independent in the raster pipeline for finer control.
- Preserve physically-based default (intensity = 1.0 → physically derived values). Avoid extra
SunLight
APIs (e.g., separate color). For stylized/colored/extraplanetary disks, recommend a custom skybox or compute-to-cubemap; the atmosphere can composite over via dual-source blending. - Disk intensity falloff: Physically motivated but likely low visual impact—defer as a later experiment to avoid API bloat.
- I sppreciated the forward-compatibility, correct fwidth AA, and center NaN fix. Feels robust and production-ready. Keep defaults to avoid breakage while offering customization; don’t over-expose knobs.
- Directional light: For Solari, tying shadow blur to disk size makes sense. Consider adding angular size directly to
DirectionalLight
or introducingDirectionalDiskLight
for generic infinite disk lights. For the Moon, prefer cubemap/skybox. - Please rename the component to
SunDisk
(it’s not a new light type). I’ll approve after the rename. - Sun disk rendering is niche; now that it’s decoupled from the atmosphere, consider generic use cases. Ensure users can disable the built-in disk and supply their own; sensible defaults should cover most needs.
let sun_intensity = (*light).intensity; | ||
if sun_angular_size > 0.0 && sun_intensity > 0.0 { | ||
let factor = 1 - smoothstep(sun_angular_size * 0.5 - w, sun_angular_size * 0.5 + w, angle_to_sun); | ||
let sun_solid_angle = (sun_angular_size * sun_angular_size) * 0.25 * PI; |
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.
This is the same expression as before, even though the PR description/comment above suggests otherwise.
FRAC_PI = 1 / PI
0.25 = 1 / 4
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.
It's precisely reciprocal, no? 4/pi vs pi/4. :)
} | ||
|
||
impl SunLight { | ||
pub const SUN: SunLight = SunLight { |
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.
typing out SunLight:SUN
or SunDisk:SUN
feels a bit awkward, but I don't have a better idea. maybe SunDisk:EARTH
? since this is exactly 1 AU away? I am adding martian atmosphere eventually so there can be a SunDisk:MARS
too that has different values.
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.
Maybe SunDisk::VIEWED_FROM_EARTH
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.
Renamed to ::EARTH
.
VIEWED_FROM_EARTH
sounds too verbose to me. "Viewed from" is the meaning of SunDisk
component, so SunDisk::EARTH
is unambiguous.
Yes, but Solari doesn't render a sun disk, so the SunDisk::intensity field shouldn't matter, right? Whereas angular_size does affect something that Solari uses.
Yes, it's a little awkward. For now just leave it. PCSS (which is what soft_shadow_size is used for) were never really working anyways.
Solari doesn't render the sun disk, so intensity doesn't matter. The standard renderer doesn't render the sun disk, or have physically accurate directional light shadows, so both angular_size and intensity don't matter. |
Hi folks! Thanks a lot for all your comments. They were extremely helpful to me, especially in narrowing down the approach and aligning it with Bevy’s vision. Also very insightful!
For now, I don’t see anything else that needs to be fixed in the PR before merging. If I’m missing anything, please let me know! @mate-h Your comments look 100% reasonable to me; I have nothing to add.
You’re right. I misunderstood this initially. Now everything looks much more reasonable to me. |
I don't have the knowledge to review the atmosphere changes, but the rest is almost ready! Just some minor doc suggestions. |
Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
Of course, once you start down that path people are going to want multiple-sun support for all their Tatooine-rendering needs . This is actually a rare case where the "two is an impossible number" rule (things are either 0, 1 or arbitrary N) might reasonably be relaxed on scientifically plausible grounds: a planet orbiting a close binary is possible, close trinary systems aren't stable. |
Objective
DirectionalLight
also lights the scene, so we need a way to skip the disk during probe baking).Addsun_disk_intensity: f32
toAtmosphereSettings
so you can smoothly hide, dim, or over‑brighten the sun disk without affecting the visible illumination of scene objects, while still allowing the sun’s radiance to scatter into the atmosphere even when the disk itself is hidden.Testing
atmosphere
example on a MacBook M1 (see showcase below).I’m new to WGSL/shader code — please point out anything silly! 🙂
Showcase
1.0 (bloom on) — normal
0.0 (bloom on) — no disk
0.01 (bloom on) — low brightness, tiny bloom effect
0.00001 (bloom on) — disk blends into the sky
10.0 (bloom on) — blinding glare
1.0 (bloom off) — normal sun with no bloom
10.0 (bloom off) — indistinguishable from 1.0 when bloom is off
Quick question for reviewers: (not part of this PR) I’ve already experimented with parameterizing the sun angular size and it works well. It could serve as a cool artistic control or let you match skies for planets that aren’t Earth. Would folks be interested in a follow‑up PR?
4× sun size