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

Proposal: Unified lights array #948

Merged
merged 9 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@ibgreen
Copy link
Contributor

ibgreen commented Mar 4, 2019

For #947

Background

  • lighting modules support a single props.lights array in addition to props.lightSettings object.

Change List

  • Cleanup pass on LightSource classes
  • Lights now include a type string field, per glTF extension spec. this allows shadertools module to safely detect light type without importing from core (would be a circular import).
  • Add tests for light sources
  • Add tests for lights shader module getUniforms.
  • Remove legacy lighting module

@ibgreen ibgreen requested review from jianhuang01 and georgios-uber Mar 4, 2019

@ibgreen ibgreen changed the base branch from master to feature/pbr-lighting Mar 4, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor

georgios-uber commented Mar 9, 2019

Ping ping

@georgios-uber georgios-uber force-pushed the feature/pbr-lighting branch from f6ef7ce to 7e3ca56 Mar 9, 2019

@ibgreen ibgreen changed the base branch from feature/pbr-lighting to master Mar 13, 2019

@ibgreen ibgreen force-pushed the ib/unified-lights branch 2 times, most recently from 840fae9 to a935427 Mar 13, 2019

if ('lights' in opts) {
const lightSources = {pointLights: [], directionalLights: []};
for (const light of opts.lights || []) {
if (light.position) {

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 14, 2019

Contributor

this is not safe if we want to make light to have generic props in the short future. We can't say it is a point light when having a position prop, same as directional light. Actually I prefer the old solution, if user wants to extend the light source module, they can have their own light source classes and won't mess up with this default one.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Author Contributor

Yes I see your point. I do find it inefficient that the user has to "sort" the light objects into different arrays.

I think that the user should just be able to put any classes inheriting from the base Light in the same array.

I think it should still be possible to make it equally extensible. Happy to implement it differently if you agree with that idea.

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 14, 2019

Contributor

No problem, got your idea. Just implement it differently. Thanks!

@ibgreen ibgreen force-pushed the ib/unified-lights branch from a935427 to d9550ac Mar 15, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 15, 2019

Coverage Status

Coverage increased (+0.7%) to 44.789% when pulling c4a487d on ib/unified-lights into 76058ce on master.

ibgreen added some commits Mar 15, 2019

@ibgreen ibgreen requested review from Pessimistress and tsherif Mar 15, 2019


// Helper: Extracts attenuation from either `props.attenuation`` or `props.intensity``
// Supports both sophisticated light model and the classic intensity prop
_getAttenuation(props) {

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 15, 2019

Contributor

put attenuation function in base light class is kind of wired, because it won't apply to ambient and directional light sources

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 15, 2019

Author Contributor

Very true. What doesn't come across in this PR is that it also applies to SpotLights ( I cut those from this PR and placed in a follow up glTF integrartion PR).

Alternatives could be to duplicate the method in Point and Spot, or create one more base class, or perhaps make SpotLight inherit from PointLight:

             Light
       /       |       \
Ambient  Directional AttenuatedLight
                      /             \
                 PointLight        SpotLight
             Light
       /       |       \
Ambient  Directional PointLight
                          \
                        SpotLight

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 15, 2019

Author Contributor

Moved into PointLight. SpotLight can inherit from PointLight so it will work nicely.

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 15, 2019

Contributor

Make SpotLight inherit from PointLight is a better solution, because SpotLight really is just a PointLight with limited emitting directions.

@ibgreen ibgreen merged commit e4e16a7 into master Mar 15, 2019

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 44.789%
Details
license/cla Contributor License Agreement is signed.
Details

@ibgreen ibgreen deleted the ib/unified-lights branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.