Supporting reversed ranges in 3d scenes #1940 #3071

archmoj · 2018-10-03T21:30:35Z

This PR would allow users to display 3D data in plotly.js scenes that may have one or multiple reversed ranges.
The options such as autorange:'reversed' and/or range:[larger, smaller] could be applied in this regard.
Most of the fixes to expand this capability are performed at vertex and fragment shaders.

@etpinard @alexcjohnson

etpinard · 2018-10-03T22:34:51Z

package.json

    "gl-select-box": "^1.0.2",
    "gl-spikes2d": "^1.0.1",
-    "gl-streamtube3d": "^1.0.0",
-    "gl-surface3d": "^1.3.5",
+    "gl-streamtube3d": "git://github.com/archmoj/gl-streamtube3d.git#f4f5a63ffb19704a1ab1cadcc7aa848d7b68f7bf",


from gl-vis/gl-streamtube3d@master...archmoj:rev-bounds

etpinard · 2018-10-03T22:35:08Z

package.json

    "gl-plot2d": "^1.3.1",
-    "gl-plot3d": "^1.5.5",
+    "gl-plot3d": "git://github.com/archmoj/gl-plot3d.git#ce601a50cd3057899f6074f8ea5fcf0ab77f417c",


gl-vis/gl-plot3d@master...archmoj:rev-bounds

There is also changes to gl-axes3d as a dependency of gl-plot3d here: gl-vis/gl-axes3d@master...archmoj:rev-bounds

etpinard · 2018-10-03T22:39:05Z

@archmoj Nice job! That's a lot of work for a first PR 💪

Now, would you mind sharing links (like I started doing above) to the patches you made in the gl-vis modules? Thank you!

antoinerg · 2018-10-03T22:57:49Z

package.json

@@ -70,21 +70,21 @@
    "es6-promise": "^3.0.2",
    "fast-isnumeric": "^1.1.1",
    "font-atlas-sdf": "^1.3.3",
-    "gl-cone3d": "^1.1.0",
+    "gl-cone3d": "git://github.com/archmoj/gl-cone3d.git#823cabfbb19d8b457730320010cb58df748ff9ee",


gl-vis/gl-cone3d@master...archmoj:rev-bounds

Why only the pick shaders here? Does triangle-fragment.glsl need to same discard statements?

Good question. In the previous version the checks for ranges were commented. When I added the checks similar to the other shaders the cones did not show up. Also for backward compatibility I thought there may be no need for them. But as you mentioned on another comment the are other issues with cones. So I will investigate this.

Also added discard logic for cone triangle-fragment.glsl as well as streamtubes.

archmoj · 2018-10-03T23:04:51Z

package.json

    "gl-heatmap2d": "^1.0.4",
-    "gl-line3d": "^1.1.2",
+    "gl-line3d": "git://github.com/archmoj/gl-line3d.git#681398a5477f64cfd92ff7494215d5b6854e834b",


gl-vis/gl-line3d@master...archmoj:rev-bounds

It might be nice to turn your new discard logic into a reusable module that we could import in (all?) our shaders using the glslify syntax.

This comment is non-blocking, but this could be a nice way for @archmoj to familiarize himself with glslify.

The outOfBound function is now located in a separate glsl file (in the local path) which is included in the shaders by glslify. Thanks for the hint : )

archmoj · 2018-10-03T23:05:17Z

package.json

    "gl-contour2d": "^1.1.4",
-    "gl-error3d": "^1.0.7",
+    "gl-error3d": "git://github.com/archmoj/gl-error3d.git#d09c224fe25671fcd588a8c803fe83129156fbab",


gl-vis/gl-error3d@master...archmoj:rev-bounds

I think I remember Mikola telling me that if-statements in shaders can be slow on some hardware. It might be worth combining these 3 if-statements into one with 2 OR operator ||.

Now the checks are performed with 2 OR operator ||.

archmoj · 2018-10-03T23:08:08Z

package.json

    "gl-mat4": "^1.2.0",
-    "gl-mesh3d": "^2.0.0",
+    "gl-mesh3d": "git://github.com/archmoj/gl-mesh3d.git#5828f6255d8c5360631d5feec65801bb104a88e8",


gl-vis/gl-mesh3d@master...archmoj:rev-bounds

archmoj · 2018-10-03T23:08:27Z

package.json

    "gl-pointcloud2d": "^1.0.1",
-    "gl-scatter3d": "^1.0.11",
+    "gl-scatter3d": "git://github.com/archmoj/gl-scatter3d.git#b649f8fb619242525b64ccf1def62cb2ed3f224f",


gl-vis/gl-scatter3d@master...archmoj:rev-bounds

archmoj · 2018-10-03T23:09:00Z

package.json

-    "gl-streamtube3d": "^1.0.0",
-    "gl-surface3d": "^1.3.5",
+    "gl-streamtube3d": "git://github.com/archmoj/gl-streamtube3d.git#f4f5a63ffb19704a1ab1cadcc7aa848d7b68f7bf",
+    "gl-surface3d": "git://github.com/archmoj/gl-surface3d.git#2455aad56d3727b940fad8d02727806abca0fdf4",


gl-vis/gl-surface3d@master...archmoj:rev-bounds

archmoj · 2018-10-03T23:09:58Z

package.json

@@ -70,21 +70,21 @@
    "es6-promise": "^3.0.2",
    "fast-isnumeric": "^1.1.1",
    "font-atlas-sdf": "^1.3.3",
-    "gl-cone3d": "^1.1.0",
+    "gl-cone3d": "git://github.com/archmoj/gl-cone3d.git#823cabfbb19d8b457730320010cb58df748ff9ee",


gl-vis/gl-cone3d@master...archmoj:rev-bounds

alexcjohnson · 2018-10-04T12:52:46Z

src/plots/gl3d/scene.js

@@ -383,6 +383,13 @@ function computeTraceBounds(scene, trace, bounds) {
    }
 }

+function isCloseToZero(a) {
+    if(Math.abs(a) > Number.MIN_VALUE) { // the smallest positive numeric value representable in JavaScript


I suppose that's a looser condition than if(a !== 0) but the numbers you're subtracting need to already be extremely small in order for their difference to be exactly Number.MIN_VALUE. I suspect we actually want to be substantially looser than that.

Exactly how that should happen I'm not sure, but I think the criterion should be something that depends on the magnitude of the numbers, to the effect of "there should be at least N representable numbers between a and b" where N is maybe 100.

see eg #179 and #1070

@archmoj did adding this isCloseToZero fix a particular bug? If so, could you provide the problematic "data" and "layout" ?

Thanks for your detailed review and great input.
Because this PR touched on many shader programs, in the reviewed version of this PR, I decided to discard closeToZero range checking. This way it would be easier to investigate if other modifications may have any side effect.
Though we can consider this info in another PR.

This way it would be easier to investigate if other modifications may have any side effect.

Great thinking. Thanks!

etpinard · 2018-10-04T19:31:14Z

I just made @archmoj and @antoinerg members of https://github.com/gl-vis. No need to push to forks ever again.

etpinard

@archmoj Very solid work! I didn't think you would have to patch every 3D gl-vis module to get this done, but yeah, this is great!

Next stop,

provide three "data" / "layout" JSON mocks. As your work touches every gl3d trace type, we'll need to make sure to test reversed axes for all those trace types, so I'm thinking we should add:
- one mock with reversed x/y/z range axes and 4 traces: 1 scatter3d with mode: 'markers+lines+text', 1 surface, 1 mesh3d and 1 streamtube trace
- one mock with x/y/z axes with autorange: 'reversed' (but no set ranges)
- one mock with reversed x/y/z axes and 1 cone trace. Cone traces don't behave well in our "old" image test container, so they are skipped during our "normal" test runs here.

Since, you can't run the image test locally (nor generate baseline images), you should commit the JSON mocks and collect the CI artefacts.

Let me know if you have any questions!

etpinard · 2018-10-04T20:06:26Z

package.json

    "gl-contour2d": "^1.1.4",
-    "gl-error3d": "^1.0.7",
+    "gl-error3d": "git://github.com/archmoj/gl-error3d.git#d09c224fe25671fcd588a8c803fe83129156fbab",


I think I remember Mikola telling me that if-statements in shaders can be slow on some hardware. It might be worth combining these 3 if-statements into one with 2 OR operator ||.

etpinard · 2018-10-04T20:08:16Z

package.json

@@ -70,21 +70,21 @@
    "es6-promise": "^3.0.2",
    "fast-isnumeric": "^1.1.1",
    "font-atlas-sdf": "^1.3.3",
-    "gl-cone3d": "^1.1.0",
+    "gl-cone3d": "git://github.com/archmoj/gl-cone3d.git#823cabfbb19d8b457730320010cb58df748ff9ee",


Why only the pick shaders here? Does triangle-fragment.glsl need to same discard statements?

etpinard · 2018-10-04T20:11:09Z

package.json

    "gl-heatmap2d": "^1.0.4",
-    "gl-line3d": "^1.1.2",
+    "gl-line3d": "git://github.com/archmoj/gl-line3d.git#681398a5477f64cfd92ff7494215d5b6854e834b",


It might be nice to turn your new discard logic into a reusable module that we could import in (all?) our shaders using the glslify syntax.

This comment is non-blocking, but this could be a nice way for @archmoj to familiarize himself with glslify.

archmoj · 2018-10-09T01:19:31Z

package.json

@@ -70,21 +70,21 @@
    "es6-promise": "^3.0.2",
    "fast-isnumeric": "^1.1.1",
    "font-atlas-sdf": "^1.3.3",
-    "gl-cone3d": "^1.1.0",
+    "gl-cone3d": "git://github.com/archmoj/gl-cone3d.git#93879d5078596f8ce27ef3d848d4ec0493faa029",


gl-vis/gl-cone3d@master...archmoj:rev-bounds

No need to paste new link whenever you pushed a new commit. The "compare" view on github will update accordingly.

archmoj · 2018-10-09T01:19:54Z

package.json

    "gl-contour2d": "^1.1.4",
-    "gl-error3d": "^1.0.7",
+    "gl-error3d": "git://github.com/archmoj/gl-error3d.git#eccf97514df079aea0eb30fca62dfc13379096b6",


gl-vis/gl-error3d@master...archmoj:rev-bounds

archmoj · 2018-10-09T01:20:07Z

package.json

    "gl-heatmap2d": "^1.0.4",
-    "gl-line3d": "^1.1.2",
+    "gl-line3d": "git://github.com/archmoj/gl-line3d.git#7019b0c682ce39e1016f33813ac0a40a37bcc7a3",


gl-vis/gl-line3d@master...archmoj:rev-bounds

archmoj · 2018-10-09T01:20:23Z

package.json

    "gl-mat4": "^1.2.0",
-    "gl-mesh3d": "^2.0.0",
+    "gl-mesh3d": "git://github.com/archmoj/gl-mesh3d.git#ec915f4996a606718a7c2a41f812363faa4db163",


gl-vis/gl-mesh3d@master...archmoj:rev-bounds

archmoj · 2018-10-09T01:20:43Z

package.json

    "gl-pointcloud2d": "^1.0.1",
-    "gl-scatter3d": "^1.0.11",
+    "gl-scatter3d": "git://github.com/archmoj/gl-scatter3d.git#6a4d678e128c3299df082073799387dec04ec8e5",


gl-vis/gl-scatter3d@master...archmoj:rev-bounds

archmoj · 2018-10-09T01:21:12Z

package.json

    "gl-select-box": "^1.0.2",
    "gl-spikes2d": "^1.0.1",
-    "gl-streamtube3d": "^1.0.0",
-    "gl-surface3d": "^1.3.5",
+    "gl-streamtube3d": "git://github.com/archmoj/gl-streamtube3d.git#5f2cf5e866865d32cc6b9058247c627af27549ac",


gl-vis/gl-streamtube3d@master...archmoj:rev-bounds

archmoj · 2018-10-09T01:21:25Z

package.json

-    "gl-streamtube3d": "^1.0.0",
-    "gl-surface3d": "^1.3.5",
+    "gl-streamtube3d": "git://github.com/archmoj/gl-streamtube3d.git#5f2cf5e866865d32cc6b9058247c627af27549ac",
+    "gl-surface3d": "git://github.com/archmoj/gl-surface3d.git#6d16d39cc858215a3df5a6c61dbcc19bd8a043b2",


gl-vis/gl-surface3d@master...archmoj:rev-bounds

etpinard · 2018-10-09T17:28:55Z

test/image/mocks/gl3d_cone-newplot_reversed_ranges.json

-    }
-  }
-}
+{"data":[{"x":[0.2],"y":[0.4],"z":[-0.6],"u":[1],"v":[1],"w":[0],"type":"cone"},{"x":[0.4],"y":[-0.6],"z":[0.2],"u":[1],"v":[0],"w":[1],"type":"cone"},{"x":[-0.6],"y":[0.2],"z":[0.4],"u":[0],"v":[1],"w":[1],"type":"cone"},{"x":[-0.2],"y":[-0.4],"z":[0.6],"u":[-1],"v":[-1],"w":[0],"type":"cone"},{"x":[-0.4],"y":[0.6],"z":[-0.2],"u":[-1],"v":[0],"w":[-1],"type":"cone"},{"x":[0.6],"y":[-0.2],"z":[-0.4],"u":[0],"v":[-1],"w":[-1],"type":"cone"}],"layout":{"height":758,"width":1310,"title":"Cone objects with Y-axis using autorange: 'reversed'","scene":{"yaxis":{"autorange":"reversed"},"camera":{"eye":{"x":1,"y":-1.5,"z":1.25}}}}}


Those new mock are very nice though. Thanks very much!

When try running image tests (on CircleCi) it showed syntax errors. I thought removing the white spaces may help pass the test. But I personally like the version which is human readable. If prefer I could fix the json mocks?

Hmm. The only syntax tests we run on JSON mocks are:

assert that their file name have no upper case

assert that they don't contain trailing newlines

So, I suspect your json files had trailing newlines. You can (and should) run those test locally with npm run test-syntax.

Would you mind reformatting those files? Thanks!

The new json mocks are reformatted.

etpinard · 2018-10-09T17:34:17Z

@archmoj

Having all those

 #pragma glslify: outOfRange = require(./reversed-scenes-out-of-range.glsl)

isn't exactly what I meant in #3071 (comment). Would you be interested in publishing this helper to npm?

Looks like you did address my

I think I remember Mikola telling me that if-statements in shaders can be slow on some hardware. It might be worth combining these 3 if-statements into one with 2 OR operator ||.

comment, but I'm curious, did swapping in the || operator make a difference in terms of performance?

archmoj · 2018-10-15T04:38:37Z

The rev-bound branches are now available on the gl-vis (rather than archmoj forks).
Regarding the comment now all the modules are using new glsl-out-of-range npm module as a new dependency.

…igned when merged with master

etpinard · 2018-10-24T16:33:02Z

Closing in favour of #3141

archmoj added 5 commits October 2, 2018 14:29

fixes for reversed bounds in 3d scenes

875067b

syntax, removed white spaces

5e4b75b

including changes to gl-axes3d which is in gl-plot3d

d338aa3

using new version of gl-axes3d

e65f346

gl-axis3d fix

ab18276

etpinard reviewed Oct 3, 2018

View reviewed changes

etpinard added bug something broken status: in progress labels Oct 3, 2018

antoinerg reviewed Oct 3, 2018

View reviewed changes

archmoj commented Oct 3, 2018

View reviewed changes

alexcjohnson reviewed Oct 4, 2018

View reviewed changes

etpinard reviewed Oct 4, 2018

View reviewed changes

archmoj added 8 commits October 4, 2018 18:05

added json mocks to collect test images

95aa008

refactoring if statements in shaders and scene

60ee80d

mocks to generate baseline images with CircleCi

7fa34f6

removed spaces from json mocks

0414976

fixed a syntax error in json streamtubes mock and added one png baseline

f65b388

added streamtube baseline image

2a72dac

using glslify for out-of-bound function

a8d0dab

correct streamtubes and cones translation

1b1a827

archmoj commented Oct 9, 2018

View reviewed changes

etpinard reviewed Oct 9, 2018

View reviewed changes

archmoj added 2 commits October 14, 2018 13:25

reformat json mocks

292d34b

using npm new module glsl-out-of-range v 1.0.1

7fc8f23

archmoj added status: reviewable and removed status: in progress labels Oct 15, 2018

archmoj added 3 commits October 15, 2018 13:55

using glsl-out-of-range v. 1.0.2 from a plotly repo

a77b9fd

resolved conflict with latest gl-plot3d and gl-axes3d

8eb570a

updated baseline images with axes alignments only - ticks would be al…

b0c1cba

…igned when merged with master

using latest gl-vis modules

f646d3f

archmoj mentioned this pull request Oct 23, 2018

Supporting reversed ranges in 3d scenes (final) #3141

Merged

etpinard closed this Oct 24, 2018

etpinard deleted the rev-bounds branch October 24, 2018 16:33

Supporting reversed ranges in 3d scenes #1940 #3071

Supporting reversed ranges in 3d scenes #1940 #3071

Conversation

archmoj commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 3, 2018

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

etpinard commented Oct 4, 2018 • edited Loading

etpinard left a 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

etpinard Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 9, 2018 • edited Loading

archmoj commented Oct 15, 2018

etpinard commented Oct 24, 2018

etpinard commented Oct 4, 2018 •

edited

Loading

etpinard Oct 9, 2018 •

edited

Loading

etpinard commented Oct 9, 2018 •

edited

Loading