Pass lights into the shader in `drawObject.ts`. #11

armcburney · 2018-05-02T00:17:03Z

Changes

Closes Pass lights into to the shader in drawObject.ts #10
Allows you to pass in multiple light sources to the Renderer class, and change them dynamically at runtime (i.e., with the addLight and removeLight methods)

Examples

One Light

Two Lights

davepagurek · 2018-05-02T00:23:19Z

src/renderer/commands/drawObject.ts

 */
 export function drawObject(
-    regl: REGL.regl
+    regl: REGL.regl,
+    numLights: number = 1,


Do you think we can make numLights be a prop we pass in? so like the regl.prop( ... ) things from before. regl.prop('something') is a shortcut for (context, props, batchId) => props['something'], so we can maybe use a function like that to do the numLights assertion

davepagurek · 2018-05-02T00:25:06Z

src/renderer/commands/drawObject.ts

-            'lightIntensities[0]': 256,
-            'lightColors[0]': [1, 1, 1]
+            numLights: numLights,
+            ...buildLightMetadata(numLights, maxLights)


Can we also add information about lights to the parameters that get passed in? so like defining an interface for a light containing a position, intensity, and color that you can use when building metadata instead of always using multiple copies of the hardcoded light I had before

davepagurek · 2018-05-02T00:25:51Z

src/renderer/commands/drawObject.ts

- * Shader to draw a single object with Phong shading to the screen
+ * Shader to draw a single object with Phong shading to the screen.
+ *
+ * @param {REGL.regl} regl: The regl object we're drawing.


nit: regl is like a factory to build a function to draw an object rather than the object being drawn

davepagurek · 2018-05-02T00:26:11Z

src/renderer/commands/drawObject.ts

+
+    return visibleLightsJSON
+        .concat(nonVisibleLightsJSON)
+        .reduce((accum: {}, obj: {}) => { return { ...accum, ...obj }; }, {});


pbardea · 2018-05-02T01:57:59Z

src/renderer/commands/drawObject.ts

 ): REGL.DrawCommand<REGL.DefaultContext, DrawObjectProps> {
+    if (numLights > maxLights) {
+        throw new RangeError('numLights must be less than or equal to maxLights.');


Can we put the actual number of lights at the end of the error message? And maybe the requested number of lights?
I'm not sure if it's bad practice to make the error message dynamic like that, but I think it would be useful to have
"numLights (24) must be less than or equal to maxLights (20)"

armcburney · 2018-05-02T20:25:08Z

src/renderer/Renderer.ts

+     * @param {REGL.Vec3} lightPositions A vector of length 3 denoting the x, y, z point where the light resides in the
+     *     vector-space.
+     * @param {REGL.Vec3} lightColors A vector of length 3 denoting _.
+     * @param {REGL.Vec3} lightIntensities A vector of length 3 denoting _.


Could use some help with these descriptions @davepagurek 😳

lightColor is a vec3 representing the r, g, and b values of the light's color, where each channel is in [0, 1]

lightIntensity is a number affecting specular reflection where a lower number produces a softer, larger reflection and a higher number produces a smaller, harder reflection

armcburney · 2018-05-02T20:26:23Z

src/renderer/commands/drawObject.ts

@@ -64,7 +66,7 @@ export function drawObject(
        frag: `
            precision mediump float;

-            const int MAX_LIGHTS = 1; // TODO(davepagurek) [#10] Increase this to allow more lights
+            const int MAX_LIGHTS = ${regl.prop('maxLights')};


This is currently giving me an error. I'm a bit unfamiliar with regl, so I'd appreciate any help @davepagurek.

This is because regl.prop('maxLights') returns a function, not a value. I think you just want maxLights directly since it's passed in at the shader's compile time

so actually I think keeping maxLights as a second argument in addition to a regl instance is good

davepagurek · 2018-05-02T21:43:09Z

src/renderer/interfaces/LightMetadata.ts

+import REGL = require('regl');
+
+export interface LightMetadata {
+    lightPositions: REGL.Vec3[];


since this is the data for one light (maybe just call it Light?), these properties should be singular, i.e. lightPosition: REGL.Vec3

Sounds good. 👍

davepagurek · 2018-05-02T21:44:08Z

src/renderer/interfaces/LightMetadata.ts

+
+export const blankLightMetadata: LightMetadata = {
+    lightPositions: [0, 0, 0],
+    lightIntensities: [0, 0, 0],


Intensity should just be a scalar instead of an array. Unfortunately it looks like typescript is bad at actually typechecking REGL.anything types

yeah, my linter was complaining. Should it be lightIntensities or just lightIntensity then?

just the singular I think

davepagurek · 2018-05-02T21:46:03Z

src/renderer/commands/drawObject.ts

+    const visibleLightsJSON: {}[] = range(maxLights).map((index: number) => {
+        return {
+            [`lightPositions[${index}]`]: (_context, props, _batch_id) => {
+                const light: LightMetadata = props.lights[index];


I think technically the type for light here would be LightMetadata | undefined

…`Renderer`.

armcburney · 2018-05-02T22:54:27Z

tests/Renderer/Renderer.spec.ts

+        lightIntensity: 256
+    }
+
+    xdescribe('addLight', () => {


davepagurek · 2018-05-02T23:42:56Z

src/renderer/Renderer.ts

-            })
+                indices: o.indices,
+                numLights: this.lights.length,
+                maxLights: this.maxLights,


tiny nit: we can take out maxLights here since we pass it in along with regl earlier so this prop doesn't get used

davepagurek

Just one small change taking out a prop we don't use, after that lgtm!

pbardea

[DELETED]

Pass lights into the shader in drawObject.ts.

4d4fb80

armcburney requested a review from davepagurek May 2, 2018 00:17

armcburney changed the title ~~GPass lights into the shader in drawObject.ts.~~ Pass lights into the shader in drawObject.ts. May 2, 2018

Don't use template string when there's no template variables.

fb4c21c

davepagurek reviewed May 2, 2018

View reviewed changes

pbardea reviewed May 2, 2018

View reviewed changes

armcburney mentioned this pull request May 2, 2018

Create configurable camera #16

Merged

Added ways to add and remove lights in Renderer class.

5255c53

armcburney commented May 2, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lights_in_shader

b13dbfc

davepagurek reviewed May 2, 2018

View reviewed changes

armcburney added 4 commits May 2, 2018 18:41

Updated renderer add/remove to take in Light. Added test suite for …

5abdc4f

…`Renderer`.

Fixed src/examples/render.ts by adding light to this.lights.

3209520

Mark failing tests as pending.

024db77

Updated example to include multiple light sources.

2981d7b

armcburney commented May 2, 2018

View reviewed changes

tests/Renderer/Renderer.spec.ts

lightIntensity: 256

}

xdescribe('addLight', () => {

Copy link

Member Author

armcburney May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#20

davepagurek reviewed May 2, 2018

View reviewed changes

davepagurek approved these changes May 2, 2018

View reviewed changes

Removed unused maxLights prop in Renderer class.

11e9de5

armcburney merged commit 7afbf5d into calder-gl:master May 3, 2018

armcburney deleted the lights_in_shader branch May 3, 2018 00:52

armcburney mentioned this pull request May 3, 2018

Create LICENSE #21

Merged

pbardea reviewed May 3, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass lights into the shader in `drawObject.ts`. #11

Pass lights into the shader in `drawObject.ts`. #11

armcburney commented May 2, 2018 •

edited

Loading

davepagurek May 2, 2018

davepagurek May 2, 2018

davepagurek May 2, 2018

davepagurek May 2, 2018

pbardea May 2, 2018

armcburney May 2, 2018

davepagurek May 2, 2018

armcburney May 2, 2018

davepagurek May 2, 2018

davepagurek May 2, 2018

davepagurek May 2, 2018

armcburney May 2, 2018

davepagurek May 2, 2018

armcburney May 2, 2018

davepagurek May 2, 2018

davepagurek May 2, 2018

armcburney May 2, 2018

davepagurek May 2, 2018

davepagurek left a comment

pbardea left a comment •

edited

Loading

Pass lights into the shader in drawObject.ts. #11

Pass lights into the shader in drawObject.ts. #11

Conversation

armcburney commented May 2, 2018 • edited Loading

Changes

Examples

One Light

Two Lights

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek left a comment

Choose a reason for hiding this comment

pbardea left a comment • edited Loading

Choose a reason for hiding this comment

Pass lights into the shader in `drawObject.ts`. #11

Pass lights into the shader in `drawObject.ts`. #11

armcburney commented May 2, 2018 •

edited

Loading

pbardea left a comment •

edited

Loading