Pass static argument to scene2d #980

dy · 2016-09-26T22:01:25Z

Merged along with this gl-vis/gl-plot2d#4 will remove image of picked data from rendered result

etpinard · 2016-09-26T22:03:05Z

Sweet! Let me test it out.

etpinard · 2016-09-26T22:27:07Z

@dfcreative I've got some news! Looks like you're almost there.

Most gl2d mocks can now be generated in a reproducible way!

Here's the list of mocks that are still problematic:

By looking at them quickly, all mocks in ⏫ have made up of one or more gl-line2d object. Maybe we'll need to bump the gl-shader dep there too?

etpinard · 2016-09-26T22:27:40Z

For reference:

etpinard · 2016-09-26T22:28:17Z

Wait @dfcreative does gl-plot2d need gl-shader@4.2.1 too?

etpinard · 2016-09-26T22:36:33Z

Ok I just checked with

and more or less the same mocks are failing intermittently:

dy · 2016-09-26T23:23:24Z

Yeah, eventually gl-shader@4.2.1 fixes errors in console, but resulting images still have bad dimensions, due to wrong canvas size etc, gotta check that the other day.

dy · 2016-09-30T15:33:46Z

Ok, 2558160 fixes dimensions, so that initial case works. Sometimes though, toImage in some reason is invoked before plot, which skews everything.

etpinard · 2016-09-30T15:36:42Z

@dfcreative great. I'll test this out this afternoon! Thanks!!

dy · 2016-09-30T15:48:58Z

src/plots/gl2d/scene2d.js

+
+    // check for resize
+    var canvas = this.canvas;
+    if(canvas.width !== pixelWidth || canvas.height !== pixelHeight) {


This is duplicate of L131 and L374. Should I put it into a method updateSize?

Ok, refactored so that now there is updateSize method

etpinard · 2016-09-30T18:32:45Z

@dfcreative looking good!

See 052dc63 where I re-generated every gl2d test image. I think they all look good now.

I'm still getting some run-to-run inconsistencies when generating images in batch (we use a concurrency of 5 in our image test routine). BUT generating them one-after-another works like charm 🎉 (at least of my machine). In commit b321cb9 we'll see if that same is true on CircleCI.

dy · 2016-09-30T19:07:43Z

What sort of inconsistencies?

The inconsistency I faced with is caused by occasional delay of initial scene.plot method, which in some reason sometimes is called after scene.toImage, so that rendered images have uninitialized content, same as real plots at the moment.
I did not manage to consistently reproduce that use-case, my laptop acclerates and initial delay becomes insufficient)

etpinard · 2016-09-30T19:14:38Z

What sort of inconsistencies?

Sorry, I should've been clearer. For example, gl2d_10 when ran in batch yield

the baseline is on the left, the generated image on the right.

etpinard · 2016-09-30T19:17:43Z

On CiricleCI, these mocks

are failing the image comparison test when ran in queue (i.e. with zero concurrency).

Note that the CircleCI machine is a little weaker than most laptops.

etpinard · 2016-09-30T19:21:57Z

@dfcreative having to run the gl2d image tests with zero concurrency would be a great start. The only drawback would be slower test runs - which isn't that big of a deal at the moment.

So, figuring out why some gl2d mocks are having issues on CircleCI would be amazing.

dy · 2016-09-30T19:26:15Z

Yes, I will check it out. Also the inconsistencies - these are the same I mentioned.

etpinard · 2016-09-30T19:29:49Z

Also the inconsistencies - these are the same I mentioned.

Great. Thanks for the info. 👍

dy · 2016-09-30T23:14:19Z

Hm. These examples, except for the gl2d_pointcloud-basic, look exactly the same as real plots. May I ask how do you get baseline images? They seem a bit taller than the rendered ones.
Baseline:

Rendered:

etpinard · 2016-10-03T13:14:33Z

May I ask how do you get baseline images?

Full docs ➡️ https://github.com/plotly/plotly.js/tree/master/test/image#c-generate-or-update-existing-baseline-image

dy · 2016-10-03T16:23:04Z

The problem is that the baseline images are taller than the default plot height I get (500px vs 450px). These mocks do not have sizes in layout, so I guess streambed may just preset own defaults or something like that.
If to preset dimensions in mocks:

"layout": {
...
        "width": 700,
        "height": 500
}

Everything works fine. Shall we?

etpinard · 2016-10-03T17:30:19Z

@dfcreative

Hmm, looks like CircleCI is still having issues - see https://circleci.com/gh/plotly/plotly.js/2597

off https://github.com/plotly/plotly.js/compare/gl2d-image-tests

dy · 2016-10-03T17:56:14Z

@monfera hi!

There seems to be a single mock failing, that is gl2d_pointcloud-basic.

The problem is when we try to read pixels from webgl in static canvas with disabled blending, they get mixed with gray-ish background or something like that.
Expected:

Actual:

What are the use-cases we may want to disable blend here? If we need solid color we can set "blend": true by default and set "color": "rgba(255, 0, 0, 1)" for example.

etpinard · 2016-10-03T17:57:01Z

On CircleCI, gl2d_multiple_subplots is generated as

monfera · 2016-10-03T18:11:52Z

@dfcreative sorry to hear it's causing testing trouble. The blending is disabled here, the purpose was an approx. 2x speedup on shader performance, which mattered a lot on the 1m point user requirement case. Admittedly the comment could be more specific. It should be possible to specify blend: true in the attributes to let this test pass. Though I had image test running problems last time, I can help with a small PR enabling blending, please let me know if I should go ahead with it.

dy · 2016-10-03T18:20:32Z

@monfera I would guess practically here can be two cases: if there is little data, then having blending enabled will give better results, whereas if there is 1m points, the precision of a single point does not matter and blending is better to be off.

Does it make any sense to detect blending dynamically?

dy · 2016-10-03T19:11:22Z

@monfera is it ok to detect blending like that 0efb54e?
I am not sure about magic numbers, the guess is if there are more than 100 points then disable blend.

monfera · 2016-10-03T19:12:29Z

@dfcreative it's possible as we already have a handle on how many points are visible at any one time. (PS. just saw your commit which looks for the total number of points, and in gl-scatter2d we have the point count for the currently visible, on-screen points - with sufficient zooming you can always get to a handful of points even if you start with a million.)

One thing though, there can be a big difference in the rendered output even if there are lots of points. With proper blending, it's possible to have a density cloud effect but of course it comes with shader time expense.

monfera · 2016-10-03T19:33:08Z

@dfcreative replying to your question - the user wouldn't know why there's a difference in behavior and it's not explained in the attributes doc, so it should be made explicit. I usually ask @etpinard when it comes to API changes so I'd defer on this too. Also I see that you set blend to undefined and you check for null, is it how it works out, i.e. does coerce turn undefined to null? If so, it looks good.

etpinard · 2016-10-03T19:55:26Z

src/traces/pointcloud/attributes.js

@@ -75,7 +75,7 @@ module.exports = {
        },
        blend: {
            valType: 'boolean',
-            dflt: false,
+            dflt: undefined,


we usually use null in situation like this one

etpinard · 2016-10-03T19:55:42Z

src/traces/pointcloud/convert.js

-    this.pointcloudOptions.blend = options.marker.blend;
+
+    var blend = options.marker.blend;
+    if (blend == null) {


... and (blend === null)

etpinard · 2016-10-13T18:39:35Z

Merging.

I'll add the CI test command in a separate PR.

- which are now reproducible via #980

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

- which are now reproducible via #980

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

Pass static argument to scene2d

cd525a4

etpinard added type: maintenance labels Sep 26, 2016

dy added 2 commits September 30, 2016 10:39

Merge branch 'master' of github.com:plotly/plotly.js

7f1527f

Fix static canvas dimensions

2558160

es5ify

07af2b8

dy commented Sep 30, 2016

View reviewed changes

dy added 2 commits September 30, 2016 12:03

Factor out canvas resize

e07201a

Make eslint happy

4fb0b19

Add dimensions to mocks

973d6ec

Detect blending dynamically

0efb54e

dy added 2 commits October 3, 2016 15:22

Fix code for linter

8192b50

Add comment

1649b1f

etpinard reviewed Oct 3, 2016

View reviewed changes

dy added 2 commits October 3, 2016 15:57

Change undefined to null

cc4d3c7

Update gl-plot2d

1bc8d08

etpinard added status: reviewable and removed status: in progress labels Oct 13, 2016

etpinard merged commit 1bf4e3e into plotly:master Oct 13, 2016

etpinard added a commit that referenced this pull request Oct 13, 2016

test: fresh set of gl2d baselines !

4f8d6af

- which are now reproducible via #980

etpinard added a commit that referenced this pull request Oct 13, 2016

test: add support for gl2d in compare pixel test routine

aff909e

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

etpinard added a commit that referenced this pull request Oct 13, 2016

test: add support for gl2d in compare pixel test routine

3913943

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

etpinard added a commit that referenced this pull request Oct 13, 2016

test: add support for gl2d in compare pixel test routine

e322bb7

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

etpinard added a commit that referenced this pull request Oct 13, 2016

test: add support for gl2d in compare pixel test routine

1093112

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

etpinard added a commit that referenced this pull request Oct 13, 2016

test: fresh set of gl2d baselines !

13b5825

- which are now reproducible via #980

etpinard added a commit that referenced this pull request Oct 13, 2016

test: add support for gl2d in compare pixel test routine

e64b05c

- warn when gl2d are ran batch - skip over multiple subplot test on CI, see #980 for details

etpinard added this to the On-par gl2d milestone Oct 13, 2016

etpinard mentioned this pull request Oct 13, 2016

gl2d image tests (finally) #1037

Merged

dy deleted the scene2d-static branch February 15, 2017 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass static argument to scene2d #980

Pass static argument to scene2d #980

dy commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

dy commented Sep 26, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

dy Sep 30, 2016 •

edited

Loading

dy Sep 30, 2016

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

etpinard commented Sep 30, 2016 •

edited

Loading

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Oct 3, 2016

dy commented Oct 3, 2016

etpinard commented Oct 3, 2016 •

edited

Loading

dy commented Oct 3, 2016

etpinard commented Oct 3, 2016

monfera commented Oct 3, 2016 •

edited

Loading

dy commented Oct 3, 2016 •

edited

Loading

dy commented Oct 3, 2016

monfera commented Oct 3, 2016 •

edited

Loading

monfera commented Oct 3, 2016

etpinard Oct 3, 2016

etpinard Oct 3, 2016

dy Oct 3, 2016

etpinard commented Oct 13, 2016

Pass static argument to scene2d #980

Pass static argument to scene2d #980

Conversation

dy commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

dy commented Sep 26, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

dy Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

dy Sep 30, 2016

Choose a reason for hiding this comment

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

etpinard commented Sep 30, 2016 • edited Loading

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Sep 30, 2016

dy commented Sep 30, 2016

etpinard commented Oct 3, 2016

dy commented Oct 3, 2016

etpinard commented Oct 3, 2016 • edited Loading

dy commented Oct 3, 2016

etpinard commented Oct 3, 2016

monfera commented Oct 3, 2016 • edited Loading

dy commented Oct 3, 2016 • edited Loading

dy commented Oct 3, 2016

monfera commented Oct 3, 2016 • edited Loading

monfera commented Oct 3, 2016

etpinard Oct 3, 2016

Choose a reason for hiding this comment

etpinard Oct 3, 2016

Choose a reason for hiding this comment

dy Oct 3, 2016

Choose a reason for hiding this comment

etpinard commented Oct 13, 2016

dy Sep 30, 2016 •

edited

Loading

etpinard commented Sep 30, 2016 •

edited

Loading

etpinard commented Oct 3, 2016 •

edited

Loading

monfera commented Oct 3, 2016 •

edited

Loading

dy commented Oct 3, 2016 •

edited

Loading

monfera commented Oct 3, 2016 •

edited

Loading