Revised pixelRatio in 3D plots to address multiple bugs by archmoj · Pull Request #3573 · plotly/plotly.js

archmoj · 2019-02-25T02:21:21Z

Supersedes #3571.

bug fix issue 3D graph axis rendering in FF64 + mac #3387 by setting up & fixing pixelRatio in various 3d modules and on the plotly.js side to work well on iOS, FireFox as well as Chrome.
bug fix issues scatter3d can't change error bars thickness #3014 & Creating images from a 3D plot changes the width of lines #3572 to preserve line widths in save 3d plots as image button as well as CI.
removed an unused ortho argument from plot_api/subroutines.js added in Orthographic projection for 3D plots - finalist #3550.
improved ticks distances in orthographic views using latest gl-axes3d.
generated a new set of gl3d baselines.

Codepen to test the fix for #3387.
Codepen to test the fix for #3572.
Codepen to test the fix for #3014.

@plotly/plotly_js

…tly.js side to work fine on iOS and FireFox bug fix issue 3572 to preserve line widths in save 3d plots as image button as well as CI removed an unused ortho argument from plot_api subroutine improved ticks distances in orthographic views using latest gl-axes3d

archmoj · 2019-02-25T02:34:54Z

package.json

+    "gl-plot3d": "git://github.com/gl-vis/gl-plot3d.git#3580a0eedd81b9f212186caa8e7018d8016274fe",
    "gl-pointcloud2d": "^1.0.2",
-    "gl-scatter3d": "^1.1.6",
+    "gl-scatter3d": "git://github.com/gl-vis/gl-scatter3d.git#6292ada845bdcd44a8e2db2d3c5e18c96c9ac210",


https://github.com/gl-vis/gl-scatter3d/compare/debug3387-false-antialias

archmoj · 2019-02-25T02:35:25Z

package.json

+    "gl-mesh3d": "git://github.com/gl-vis/gl-mesh3d.git#31d34ba3cec063697cc3891e141532cd80d95ba3",
    "gl-plot2d": "^1.4.2",
-    "gl-plot3d": "^2.0.0",
+    "gl-plot3d": "git://github.com/gl-vis/gl-plot3d.git#3580a0eedd81b9f212186caa8e7018d8016274fe",


https://github.com/gl-vis/gl-plot3d/compare/debug3387-false-antialias

nicely done here 🥇

archmoj · 2019-02-25T02:36:15Z

package.json

+    "gl-cone3d": "git://github.com/gl-vis/gl-cone3d.git#ec71aa87233d04b543ae53854c9a1182d40fc8ac",
    "gl-contour2d": "^1.1.5",
-    "gl-error3d": "^1.0.13",
+    "gl-error3d": "git://github.com/gl-vis/gl-error3d.git#75ec29dd1dde51a697f718cdacf76e36a7bfd38c",


https://github.com/gl-vis/gl-error3d/compare/fix-false-antialias-issue3387

archmoj · 2019-02-25T02:37:11Z

package.json

-    "gl-streamtube3d": "^1.1.2",
-    "gl-surface3d": "^1.4.1",
+    "gl-streamtube3d": "git://github.com/gl-vis/gl-streamtube3d.git#ecdaa1dbbcd315ba06133aeccf870c98b993a8ae",
+    "gl-surface3d": "git://github.com/gl-vis/gl-surface3d.git#96e61eebac18e9b525f7f3e6b714291b5d1b4bad",


https://github.com/gl-vis/gl-surface3d/compare/debug3387-false-antialias

etpinard · 2019-02-25T14:50:31Z

Wow. Fantastic PR @archmoj !

[x] generated a new set of gl3d baselines.

Are there any noticeable differences in the new baselines? Glancing at them quickly didn't reveal anything, but I'd like to double-check with you.

test/image/mocks/gl3d_mesh3d_surface3d_scatter3d_line3d_error3d_log_reversed_ranges.json

etpinard · 2019-02-25T14:52:51Z

test/jasmine/tests/gl3d_plot_interact_test.js

    });

-    it('@gl should display correct hover labels (mesh3d case)', function(done) {
+    it('@noCI @gl should display correct hover labels (mesh3d case)', function(done) {


Does this test still pass locally using

(plotly.js) $ ./tasks/noci_test.sh

?

Unfortunately no. The mock is unexpectedly rotated after the click possibly related to uirevision.
But I need to figure that out.`

Yes please,

(plotly.js) $ ./tasks/noci_test.sh

should (always 😄 ) pass.

src/traces/scatter3d/convert.js

etpinard · 2019-02-25T14:59:10Z

src/plots/gl3d/scene.js

    this.container = sceneContainer;
    this.staticMode = !!options.staticPlot;
-    this.pixelRatio = options.plotGlPixelRatio || 2;
+    this.pixelRatio = this.pixelRatio || options.plotGlPixelRatio || 2;


This may break graphs where users set config option plotGlPixelRatio:

plotly.js/src/plot_api/plot_config.js

Lines 309 to 319 in 13b925c

plotGlPixelRatio: {

valType: 'number',

dflt: 2,

min: 1,

max: 4,

description: [

'Set the pixel ratio during WebGL image export.',

'This config option was formerly named `plot3dPixelRatio`',

'which is now deprecated.'

].join(' ')

},

Orca does it here:

https://github.com/plotly/orca/blob/776d5c3f1faf7272f7eb6e1c11ba8d7fb0057734/src/component/plotly-graph/render.js#L30

using (a strange) value:

https://github.com/plotly/orca/blob/776d5c3f1faf7272f7eb6e1c11ba8d7fb0057734/src/component/plotly-graph/constants.js#L35

Oh wait nevermind, this.pixelRatio isn't set by default (correct?) so options.plotGlPixelRatio is honoured.

Yes I think so.

Ok great. Can you try generating the gl3d_* baseline using orca with a few different plotGlPixelRatio values?

You may want to modify:

plotly.js/tasks/noci_test.sh

Lines 31 to 39 in 3e15f17

test_image () {

$root/../orca/bin/orca.js graph \

$root/test/image/mocks/mapbox_* \

$root/test/image/mocks/gl3d_cone* \

--plotly $root/build/plotly.js \

--mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \

--output-dir $root/test/image/baselines/ \

--verbose || EXIT_STATE=$?

}

to get orca started easily and you might have to tweak the orca src files e.g. here:

https://github.com/plotly/orca/blob/776d5c3f1faf7272f7eb6e1c11ba8d7fb0057734/src/component/plotly-graph/constants.js#L35

or

https://github.com/plotly/orca/blob/776d5c3f1faf7272f7eb6e1c11ba8d7fb0057734/src/component/plotly-graph/render.js#L30

to pass the different plotGlPixelRatio values down to plotly.js

@etpinard Thank you so much for the links. Using orca I was able to produce gl3d_* images from this branch and compared them against the master branch. I also tweaked the pixelRatio of orca module and the configuration has an impact.

archmoj · 2019-02-25T15:31:35Z

Wow. Fantastic PR @archmoj !

[x] generated a new set of gl3d baselines.

Are there any noticeable differences in the new baselines? Glancing at them quickly didn't reveal anything, but I'd like to double-check with you.

Well the primary goal of this PR was to possibly improve rendering of 3-D plots on different browsers & OS. Although in the other PR #3571 I was able to improve the quality of CI images by doubling the pixelRatio when anti-alias is not set to true by the hardware; the CI images form this PR may not illustrate much differences namely for the axis-lines. Anyway if we were interested to also achieve better CI images one idea may be to pass a different flag or use higher pixelRatio in the CI config?

new baselines using previous dims

baseline with original width

etpinard · 2019-02-26T02:09:43Z

nyway if we were interested to also achieve better CI images one idea may be to pass a different flag or use higher pixelRatio in the CI config?

I don't think that's necessary, but we should make sure that users setting plotGlPixelRatio get more pixels. Perhaps we could add a jasmine test for that using e.g.

https://github.com/plotly/plotly.js/blob/master/test/jasmine/assets/read_pixel.js

archmoj · 2019-02-26T03:43:40Z

Wow. Fantastic PR @archmoj !

[x] generated a new set of gl3d baselines.

Are there any noticeable differences in the new baselines? Glancing at them quickly didn't reveal anything, but I'd like to double-check with you.

Regarding your question changes are noticeable in the following examples i.e. mainly related to surface contour lines and error bars.
https://github.com/plotly/plotly.js/pull/3573/files?short_path=d205ea8#diff-d205ea8d76e3e1a099b79bd08640c05b

https://github.com/plotly/plotly.js/pull/3573/files?short_path=5ae25d0#diff-5ae25d025346966db02254e38ac31363

https://github.com/plotly/plotly.js/pull/3573/files?short_path=1a569b4#diff-9c01fba6d75f01f7dfd92c50b2382ce3

etpinard · 2019-02-26T17:43:26Z

Going to slip this in 1.45.0.

This is the last PR for 1.45.0 I promise 😄

archmoj · 2019-02-26T21:15:47Z

Going to slip this in 1.45.0.

This is the last PR for 1.45.0 I promise
@etpinard thanks
The modified test are passed on the CI and locally.

etpinard · 2019-02-26T21:17:22Z

💃 💃 💃

archmoj added bug something broken status: reviewable labels Feb 25, 2019

This was referenced Feb 25, 2019

Detect and increase pixelRatio in 3D plots when anti-alias is not enabled by hardware #3571

Closed

config option plotGlPixelRatio changes size of markers #3246

Closed

archmoj commented Feb 25, 2019

View reviewed changes

etpinard reviewed Feb 25, 2019

View reviewed changes

test/image/mocks/gl3d_mesh3d_surface3d_scatter3d_line3d_error3d_log_reversed_ranges.json Outdated Show resolved Hide resolved

etpinard reviewed Feb 25, 2019

View reviewed changes

src/traces/scatter3d/convert.js Show resolved Hide resolved

etpinard reviewed Feb 25, 2019

View reviewed changes

archmoj added 2 commits February 25, 2019 11:48

reset the image widths to their original values

2a80673

new baselines using previous dims

removed noCI flag from the jasmine test & reset one more mock width

9ad6634

baseline with original width

edited one CI jasmine test in order no to rotate camera before click

012625d

etpinard added this to the v1.45.0 milestone Feb 26, 2019

archmoj added 6 commits February 26, 2019 13:27

fixed typo in gl-mesh3d and edited mesh3d test

4542e8d

bypass test

85233b2

bump half of the modules

27cfcab

bump second half of gl modules

7f48ecf

edit isosurface noci test

9586f78

noCI flag back

5096f4f

archmoj merged commit 50da961 into master Feb 26, 2019

etpinard deleted the fixup-pixelratio-3d branch February 26, 2019 21:25

archmoj mentioned this pull request Mar 15, 2019

Bump gl-surface3d 1.4.3 #3641

Merged

archmoj mentioned this pull request Jan 30, 2020

Fix hover for mesh3d, isosurface and volume #4534

Merged

	plotGlPixelRatio: {
	valType: 'number',
	dflt: 2,
	min: 1,
	max: 4,
	description: [
	'Set the pixel ratio during WebGL image export.',
	'This config option was formerly named `plot3dPixelRatio`',
	'which is now deprecated.'
	].join(' ')
	},

	test_image () {
	$root/../orca/bin/orca.js graph \
	$root/test/image/mocks/mapbox_* \
	$root/test/image/mocks/gl3d_cone* \
	--plotly $root/build/plotly.js \
	--mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \
	--output-dir $root/test/image/baselines/ \
	--verbose \|\| EXIT_STATE=$?
	}

Uh oh!

Conversation

archmoj commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Feb 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

archmoj commented Feb 25, 2019

Uh oh!

etpinard commented Feb 26, 2019

Uh oh!

archmoj commented Feb 26, 2019

Uh oh!

etpinard commented Feb 26, 2019

Uh oh!

archmoj commented Feb 26, 2019

Uh oh!

etpinard commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

archmoj commented Feb 25, 2019 •

edited

Loading