Adding light position configurability for `surface` and `mesh3d`, and general lighting configurability for the latter #556

monfera · 2016-05-20T21:29:48Z

Part of the fix to #552

The fix isn't actually to the 'plotly.js' repo, but rather, the dependencies are changed such that epsilon values can be passed on to them. The alternative fix in the dependencies (changing the epsilon in the normals repo) was deemed less good because the epsilon is better be configurable in the dependencies.

PRs in dependent issues:
mikolalysenko/normals#2
gl-vis/gl-mesh3d#5

The other part of the fix is even less to do with plotly.js (except bumping the dependency version number): gl-vis/gl-mesh3d@b901f89

Adding epsilon configuration in order to pass it on to newly modified dependencies that now take these new params. The default values are set such that 3D mesh models with minute triangles are also properly included in the GL specular light reflection calculation.

WIP because:

it assumes the PR acceptance, merge in and npm update of a dependency; which in turn assumes a similar change to its own dependency
attribute and image test cases are not yet added
anything I forgot?

monfera · 2016-05-20T21:57:35Z

As of now, the CI check obviously fails for the reason that the package dependencies haven't yet been bumped and pushed to the NPM repo so it doesn't find gl-mesh3d v1.1.0.

monfera · 2016-05-20T21:58:53Z

Adding @mikolalysenko @etpinard @bpostlethwaite

etpinard · 2016-05-25T13:59:16Z

src/traces/surface/attributes.js

@@ -182,6 +206,20 @@ module.exports = {
            min: 0.00,
            max: 5.00,
            dflt: 0.2
+        },
+        vertexnormalsepsilon: {


maybe vertexnormal and facenormal would suffice?

@etpinard it's quite specific to to the epsilon error terms in the normals calculation and these names would be suggestive of passing on the normals directly.

OK. Agreed. I won't be the most used attribute nor is it the most obvious ; might as well be verbose. 👍

etpinard · 2016-05-25T14:05:42Z

@monfera looks like your patch led to image test diffs:

Is that on purpose?

etpinard · 2016-05-25T14:11:25Z

@monfera great work 🎉

Thanks for bringing our mesh3d trace type up to its full potential.

Before merging this in:

agree on vertex normal and face normal epsilon attribute names
update the baseline images that generated diffs
add a image test mock showing off custom lightposition, and vertex/face normal epsilions.
add description fields to all added attributes

monfera · 2016-05-25T14:58:18Z

@etpinard yes, I knew about the image diff but haven't yet pushed the commit for its solution. Also I agree with your list and push commits, and pls. see my reply on the naming.

etpinard · 2016-05-25T17:53:48Z

src/traces/surface/attributes.js

        },
        roughness: {
            valType: 'number',
            role: 'style',
            min: 0.00,
            max: 1.00,
-            dflt: 0.5
+            dflt: 0.5,
+            description: 'Alters specular reflection; the rougher the surface, the wider and less contrasty the shine.'


Wow. These descriptions are great. Thanks!

cc @cldougl @chriddyp

etpinard · 2016-05-25T17:56:52Z

Yes yes. The new Snowden image is incredible:

monfera · 2016-05-25T22:29:58Z

The included test doesn't yet cover specular reflection yet. Snowden head shows the effects of the diffuse and ambient parameters, and the mesh color parameter is also exercised.

monfera · 2016-05-26T10:25:57Z

@etpinard regarding the coercion of vertexnormalsepsilon and facenormalsepsilon: these turn out not to be used by the underlying gl-surface3d so I'm making it specific to mesh3d, in line with your original PR suggestion.

I added commits to this PR such that the directional light position is being sent to gl-surface3d, see the numerous surface png diffs, most of which are superficial (changed reflection due to different light position - formerly, the default in gl-surface3d prevailed, and now the plotly.js default prevails, which was taken from gl-mesh3d to begin with.

The different lighting cast on the yield curve highlights an issue with the specific test case. I'll look into that but it is unrelated to this PR; even in the former image, the unexpected shady bands are somewhat visible.

monfera · 2016-05-26T11:12:08Z

Snowden in a different lighting and orientation:

monfera · 2016-05-26T13:09:47Z

@etpinard I didn't want to fiddle with your PR checklist above but I think they were covered by late yesterday, and today's commits made light positions work with surface too. The epsilon values weren't needed as it uses a different specular calculation method.

etpinard · 2016-05-26T13:40:25Z

src/traces/mesh3d/attributes.js

+            dflt: 1e-6, // even the brain model doesn't appear to need finer than this
+            description: 'Epsilon for face normals calculation avoids math issues arising from degenerate geometry.'
+        }
+    }, surfaceAtts.lighting),


etpinard · 2016-05-26T13:47:19Z

@monfera

The different lighting cast on the yield curve highlights an issue with the specific test case.

Can you elaborate? If I understand correctly, lightning on surface trace isn't perfect yet?

… dependencies that now take these new params. The default values are set such that 3D mesh models with minute triangles are also properly included in the GL specular light reflection calculation.

…stly improved Snowden head)

…s. Not covered: specular and its constituent parameters.

… eye

Set a different default light position for surface vs mesh

etpinard · 2016-05-26T19:37:01Z

@monfera Fantastic work.

You win the PR of the week 🏆

monfera force-pushed the fix-brain branch from 0ad4443 to ff82577 Compare May 20, 2016 21:41

monfera mentioned this pull request May 20, 2016

Lighting doesn't work on certain 3D mesh plots #552

Closed

monfera changed the title ~~Adding epsilon configuration [WIP]~~ [WIP] Adding epsilon configuration May 20, 2016

monfera force-pushed the fix-brain branch from 836084d to ad218ba Compare May 24, 2016 11:44

monfera changed the title ~~[WIP] Adding epsilon configuration~~ [WIP] Adding epsilon and light position configuration May 24, 2016

monfera force-pushed the fix-brain branch from ad218ba to f787e2e Compare May 24, 2016 18:26

etpinard added feature something new status: reviewable labels May 25, 2016

etpinard reviewed May 25, 2016
View reviewed changes

etpinard added status: in progress and removed status: reviewable labels May 25, 2016

monfera force-pushed the fix-brain branch 2 times, most recently from 2386922 to 10a2ca7 Compare May 25, 2016 16:40

etpinard reviewed May 25, 2016
View reviewed changes

monfera changed the title ~~[WIP] Adding epsilon and light position configuration~~ Adding epsilon and light position configuration May 25, 2016

etpinard added status: reviewable and removed status: in progress labels May 25, 2016

monfera changed the title ~~Adding epsilon and light position configuration~~ Adding light position configurability for surface and mesh3d, and general lighting configurability for the latter May 26, 2016

etpinard reviewed May 26, 2016
View reviewed changes

monfera force-pushed the fix-brain branch 2 times, most recently from eddc43b to 2fc27f1 Compare May 26, 2016 15:43

monfera added 10 commits May 26, 2016 20:48

Adding epsilon configuration in order to pass it on to newly modified…

cd18bde

… dependencies that now take these new params. The default values are set such that 3D mesh models with minute triangles are also properly included in the GL specular light reflection calculation.

Adding light source configurability

afed66a

Adding attribute documentation

af2f201

Updated complex mesh3d images (unnoticable change except for the va…

c9aa437

…stly improved Snowden head)

Test: mesh color, ambient reflection and diffuse reflection parameter…

8b4c4b1

…s. Not covered: specular and its constituent parameters.

Test: directional light position and camera attributes up, center and…

694a15a

… eye

Test case changed for a surface3d image

401d3c3

Make vertexnormalsepsilon and facenormalsepsilon specific to mesh3d

d81c352

Set a different default light position for surface vs mesh

Test case changed for a surface3d image

45c9e41

Minor image updates (some of them reversals)

d300a9b

monfera force-pushed the fix-brain branch from 2fc27f1 to d300a9b Compare May 26, 2016 19:22

etpinard merged commit 12c2d8c into plotly:master May 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding light position configurability for `surface` and `mesh3d`, and general lighting configurability for the latter #556

Adding light position configurability for `surface` and `mesh3d`, and general lighting configurability for the latter #556

monfera commented May 20, 2016 •

edited

Loading

monfera commented May 20, 2016

monfera commented May 20, 2016

etpinard May 25, 2016

monfera May 25, 2016

etpinard May 25, 2016

etpinard commented May 25, 2016

etpinard commented May 25, 2016 •

edited

Loading

monfera commented May 25, 2016

etpinard May 25, 2016

etpinard May 25, 2016

chriddyp May 25, 2016

etpinard commented May 25, 2016

monfera commented May 25, 2016 •

edited

Loading

monfera commented May 26, 2016 •

edited

Loading

monfera commented May 26, 2016

monfera commented May 26, 2016

etpinard May 26, 2016

etpinard commented May 26, 2016

etpinard commented May 26, 2016

Adding light position configurability for surface and mesh3d, and general lighting configurability for the latter #556

Adding light position configurability for surface and mesh3d, and general lighting configurability for the latter #556

Conversation

monfera commented May 20, 2016 • edited Loading

monfera commented May 20, 2016

monfera commented May 20, 2016

etpinard May 25, 2016

Choose a reason for hiding this comment

monfera May 25, 2016

Choose a reason for hiding this comment

etpinard May 25, 2016

Choose a reason for hiding this comment

etpinard commented May 25, 2016

etpinard commented May 25, 2016 • edited Loading

monfera commented May 25, 2016

etpinard May 25, 2016

Choose a reason for hiding this comment

etpinard May 25, 2016

Choose a reason for hiding this comment

chriddyp May 25, 2016

Choose a reason for hiding this comment

etpinard commented May 25, 2016

monfera commented May 25, 2016 • edited Loading

monfera commented May 26, 2016 • edited Loading

monfera commented May 26, 2016

monfera commented May 26, 2016

etpinard May 26, 2016

Choose a reason for hiding this comment

etpinard commented May 26, 2016

etpinard commented May 26, 2016

Adding light position configurability for `surface` and `mesh3d`, and general lighting configurability for the latter #556

Adding light position configurability for `surface` and `mesh3d`, and general lighting configurability for the latter #556

monfera commented May 20, 2016 •

edited

Loading

etpinard commented May 25, 2016 •

edited

Loading

monfera commented May 25, 2016 •

edited

Loading

monfera commented May 26, 2016 •

edited

Loading