Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ADR 276: light sources #276
base: main
Are you sure you want to change the base?
ADR 276: light sources #276
Changes from 2 commits
3893427
5e914b1
1cee77d
c408d59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Regarding the component structure, I'd like to discuss an alternative approach based on composition rather than a single component with type discrimination:
Instead of:
Consider a composable approach:
Benefits:
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.
We debated this point, and reached the conclusion that we still much prefer to use a single component, for the following reasons:
LightSource.spotLight(myEntity, { brightness: 1000, angle: 10})
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 suggest reconsidering the use of arbitrary 0-1 values for light intensity in favor of physically-based units (lumens/m² or lux). This would provide several advantages:
Predictability: Artists and developers can reference real-world light measurements
Consistency: Light behavior becomes more predictable across different implementations and scenes
Realism: Natural light falloff can be accurately modeled using the inverse square law
Interoperability: Better alignment with industry standards and other 3D tools
We have a working implementation of this approach in the protocol files that you might want to review: light.proto
The property could be renamed to
illuminance
to better reflect its physical meaning. The actual visual impact would still be subject to the renderer's exposure and tone mapping, but the relative relationships between lights would be preserved.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 suggestion, how about we use Lumens instead, it measure the total amount of light emitted by a source. This makes it the ideal metric for describing the output of a light source, independent of where or how it is used. Lumens are inherently tied to the light source itself, while lux depends on the environment. Since we want to define properties of the light sources rather than the effect of light on a surface, lumens are a better fit, in my option.
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.
We could use Lumens of course, that would be equivalent to divide the luxs by 4*π. But you should keep using lux for directional lights since the source is practically infinity. And this differentiation is the one glTF extension uses.
glTF extension:
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.
After talking it over and looking at other platforms, we changed the value to be measured in lumens and renamed to "brightness"
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.
And luxs for directional/global?
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.
You're right, I was leaving that out bc global lights aren't in the plan for our first implementation, but it should be in the ADR.
I just included a clarification for that:
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.
Regarding light range, I suggest following GLTF's KHR_lights_punctual approach:
The range should be optional (or even more not being added now). When unspecified, light naturally SHOULD attenuate following the inverse square law (1/d²)
When specified, GLTF uses this attenuation formula after following the inverse square law, then:
Within the range of the light, attenuation should follow the inverse square law as closely as possible, although some non-quadratic falloff near the edge of the range may be used to avoid a hard cutoff
This provides:
Instead of having developers guess appropriate range values, we could:
This aligns with industry standards while maintaining physically-based behavior.
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.
We agree that the attenuation curve of light should mimic real life as closely as possible
The "range" field should serve as a hard cut-off, but not distort the attenuation curve.
It's mostly there to allow creators to optimize. The default value should be something reasonable, and most novice creators might not need to tweak it
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.
Regarding shadows, I suggest expanding this section to address several important aspects:
Instead of a simple boolean, consider these key properties:
shadows: boolean
- Whether the light casts shadowsfalse
(performance consideration)true
(essential for scene depth)Add a note that shadow rendering may vary by platform:
Consider adding guidance for implementations about shadow priority:
This approach:
See our protocol implementation for a working example of these concepts.
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.
We agree that it's better to have shadows as a simple boolean, and let each engine determine things like shadow resolution, priorities, etc.
I changed this in the document
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 suggest removing the "Limitations and Considerations" section from this ADR. Here's why:
Let each explorer/renderer decide how to handle:
The SDK can provide:
These topics deserve their own documentation:
This approach:
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’m okay with removing the Limitations and Considerations section, but I’d like to challenge this approach slightly. These limitations guide how each client should behave, ensuring consistency across implementations. Without them, we risk significant divergence in how the feature works across clients, which might shift responsibility to creators to adapt to different behaviors.
Creating a separate ADR or RFC for these constraints could work, but we’d need to align on how the protocol should remain generic while still providing a baseline for consistent behavior. I also see the value in keeping this ADR abstract to allow for experimentation and flexibility, but it’s crucial we ensure creators aren’t left with unpredictable experiences due to discrepancies between clients.
Are we ok with having that differences between clients?
If we move this to another ADR, then we need to provide a clear guidelines, tools and documentation for creators to navigate that different clients.
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 agree with both, the ADR should focus on the protocol, but sometimes the protocol is more than just the data sent via the transport, it's also about the expected behavior.
I simplified these lines to something more generic:
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.
The Global Light section needs more detailed specification. Currently, there's a gap between point/spot lights and scene-wide lighting. Consider:
A scene's global lighting should be configurable through two complementary components:
Light
on root = Override default directional (sun) lightGlobalLight
on root = Control direction and ambient settingsLight
alone = Change sun color/intensityGlobalLight
alone = Change direction/ambientAdd specification for default values:
This matches our protocol implementation while providing clear guidance for scene creators and engine implementations.
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.
Agreed, I expanded the section, and added more to the initial definition. I however added this as an additional type, rather than its own component.