new camera light feature #2943
new camera light feature #2943
Conversation
| @@ -0,0 +1,3 @@ | |||
| import {PointLight} from '@luma.gl/core'; | |||
There was a problem hiding this comment.
Time to create an effects/lighting directory?
| : COORDINATE_SYSTEM.IDENTITY, | ||
| fromCoordinateOrigin: [0, 0, 0] | ||
| }); | ||
| if (pointLight instanceof CameraLight) { |
There was a problem hiding this comment.
It's a little concerning that the effect class has to know about CameraLight for it to work. Can users not implement their own light sources?
We could defer to each light source class to calculate this, e.g.
pointLight.getPosition(parameters);There was a problem hiding this comment.
Lighting effect is owner of different light sources, I think it is reasonable for it to know each kind of light source, default behaviors and doing some jobs based on that.
User can implement their own light sources, but we need define the light source category and the basic behavior first. For example, user can implement their own CamerLight by extending from our CameraLight source, we will provide the internal camera position automatically as a predefined behavior, and user don't need understand our complicated projection system.
Point light sources stores the pre-projection position, color, and intensity ,etc. If we put this project logic inside light sources, there will be a coupling between viewport, projection system and the light source classes, which is not a good design from my perspective. Also the projected position is not the same under different viewport & zoom, then the getPosition API is even more ambiguous with those complicated logics lives together.
There was a problem hiding this comment.
Going to move viewport related logic to light source classes per offline discussion in case user need implement their own light source category.
| @@ -153,6 +153,58 @@ function calculateMatrixAndOffset({ | |||
| }; | |||
| } | |||
|
|
|||
| export function getCameraPositionFromViewport({ | |||
There was a problem hiding this comment.
Is this just duplicating code in this file? At least reuse this for project module's uniform generation?
There was a problem hiding this comment.
You should be able to import the project module and call project.getUniforms without this new utility function.
There was a problem hiding this comment.
I made this new API according to ib's review comments, I can revert this change if it is not good. Orginially I use the existing API 'getUniformsFromViewport', which way do you prefer?
There was a problem hiding this comment.
Going to revert back to 'getUniformsFromViewport' solution.
|
This discussion is getting rather detailed but once the dust settles, might be worth a quick audit of the proposed API changes/logic? Viewport and project systems are a complex part of deck, and also lighting is an area that is still under active development in both luma and deck, so it is probably worth giving this the extra thought. Also, have we considered pushing the camera light feature to 7.1? At some point a project needs to accept that certain features missed the "train" and wait on the platform for another 4-5 weeks for the next train. Perhaps this class can be done application-side if some app cannot wait? While we take the time to audit this and do it right? |
jianhuang01
force-pushed
the
camera-light
branch
from
5603bb0
to
e7f0a54
Compare
|
Don't forget to add documentation. |
Document will be a different PR, postpone that cause I want to make sure team agrees on the solution. |
Did API audit in jam meeting, as the conclusion we make CameraLight as experimental. |
For #2833
Change List