Fix fill tozero with undefined value and overlaping of fill area #2979

ErwanMAS · 2018-09-07T04:58:40Z

This is my last version

example of the current issue ( https://codepen.io/erwanmas/pen/JaymVr )

when undefined values with option fill tozerox or tozeroy
order of fill scattergl when overlap

etpinard · 2018-09-07T15:50:52Z

Thanks again @ErwanMAS for helping us out!

Judging by:

does this PR also fix #2765 ?

test/image/mocks/gl2d_fill_trace_filly.json

src/traces/scattergl/index.js

catcup 8 sept

catch up

etpinard · 2018-09-12T13:40:55Z

src/traces/scattergl/attributes.js

@@ -54,6 +54,8 @@ var attrs = module.exports = overrideAll({
    line: {
        color: scatterLineAttrs.color,
        width: scatterLineAttrs.width,
+        shape: scatterLineAttrs.shape,
+        smoothing: scatterLineAttrs.smoothing,


Ha. I see what you did there. You had to add smoothing so that you could reuse handleLineShapeDefaults in the defaults directly.

But smoothing and line.shape: 'spline' aren't implemented in scattergl, so we can't just reuse the scatter attributes and defaults. It's important to have the correct attributes declaration as these are used to generate doc pages like this one: https://plot.ly/javascript/reference/

OK ... so i will copy/paste instead .

src/traces/scattergl/index.js

src/traces/scattergl/line_shape_defaults.js

src/traces/scattergl/index.js

etpinard · 2018-09-17T15:40:02Z

@ErwanMAS thanks again for you hard work.

If I understand correctly, this PR fixes 2 bug (fill tozero with missing coordinates and overlapping fills ordering) and adds a feature (all scatter line.shape values except 'spline').

Currently,

the fill tozero bug fix looks good ✅
I still have doubts about the overlapping fills fix. It is an improvement for sure, but it doesn't resolve all our fill/ordering issues in scattergl. 🚫
the new line.shape feature looks good ✅

In its current form, this PR could only be released in the next minor (v1.42.0 which is probably two weeks away) as it adds a new feature. If you're looking to have your two bug fixes released sooner, I'd recommend opening another PR or two (one for each bug fix).

test/image/mocks/gl2d_fill_trace_tozero_order.json

catchup

- when undefined values with option fill tozerox or tozeroy - order of fill scattergl when overlap

still missing spline from scatter

…s.js

- new test with more overlap with more kind of objects

modified baseline for gl2d_fill_trace_tozero_order / gl2d_scatter_fill_self_next

add somes text traces into the baseline test

ErwanMAS · 2018-10-01T01:59:54Z

@etpinard , i released a new version of my patch .

ErwanMAS · 2018-10-01T02:19:19Z

@etpinard i updated my patch . and i added a new test case . It will fix the overlapping issues

etpinard · 2018-10-01T14:36:03Z

Thanks @ErwanMAS !

In the future, would you mind not git push -f new commits on branches associated with open PRs? This makes somewhat annoying to keep it with your work. For example, from the Github UI I can't tell which of your commits are "new". If you need to bring your branch up-to-date with the current master, use git merge master instead. Thank you!

etpinard · 2018-10-04T21:13:26Z

src/traces/scattergl/index.js

@@ -261,11 +261,8 @@ function sceneUpdate(gd, subplot) {
            var i;
            for(i = 0; i < scene.count; i++) {
                if(scene.fill2d && scene.fillOptions[i]) {
-                    // must do all fills first


Hmm. Looks like this broke the gl2d_scatter_fill_self_next mock:

The first two traces have fill: 'tonext' meaning their fills should be behind both the data[0] and data[1] markers. See corresponding SVG structure:

available here -> https://rreusser.github.io/plotly-mock-viewer/#scatter_fill_self_next

i pushed a new version .

…/baselines/gl2d_scatter_fill_self_next.png ) - add a new test with 2 draw side by side

etpinard · 2018-10-05T20:37:00Z

src/traces/scattergl/index.js

            for(i = 0; i < scene.count; i++) {
+                var we_defer_somes_draw = 0;
+                if(scene.fill2d && scene.fillOptions[i] && scene.fillOptions[i].fillmode && scene.fillOptions[i].fillmode === 'tonext') {


I think you got the fill part right 🎉

The logic is a little hard to follow; it would be nice to to ♻️ the logic from the SVG version:

plotly.js/src/traces/scatter/plot.js

Lines 95 to 105 in 88d48c5

var fillData = [];

if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' || trace.fill === 'toself' ||

(trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))

) {

fillData = ['_ownFill'];

}

if(trace._nexttrace) {

// make the fill-to-next path now for the NEXT trace, so it shows

// behind both lines.

fillData.push('_nextFill');

}

by using _nexttrace and _prevtrace which are defined during linkTraces called in scattergl here.

Moreover, looks like the error bars are drawn above the lines, SVG scatter does it the other way around.

Oh well, I'll try out a few things and hopefully that one or two commits will be enough to bring this to the finish line.

Thanks again.

@ErwanMAS I made two commits on branch etienne-scattergl-ordering which appear to make things work while reusing the SVG scatter logic, See:

075bd14

6ae6283

Do they look ok to you? If so, I'll release them along with your work in next week's v1.42.0.

@etpinard for me this seem to be good . Can you take the last version of https://github.com/plotly/plotly.js/blob/4b2b0d5d34e648e361532fe23f5dbff536792e1c/test/image/mocks/gl2d_scatter_fill_self_next_vs_nogl.json . The new version has now 4 charts .

etpinard · 2018-10-09T16:04:54Z

Ok thanks again @ErwanMAS !

I opened a new PR (#3087) with your commits from this PR along with my small improvements. Closing this PR in favour of #3087

ErwanMAS mentioned this pull request Sep 7, 2018

- fix issue scattergl and fill to zero and undefined value #2965

Closed

etpinard reviewed Sep 7, 2018

View reviewed changes

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

etpinard reviewed Sep 7, 2018

View reviewed changes

src/traces/scattergl/index.js Show resolved Hide resolved

etpinard added bug something broken status: in progress labels Sep 7, 2018

Merge pull request #1 from plotly/master

1c52fa6

catcup 8 sept

etpinard mentioned this pull request Sep 10, 2018

test case for bug #2765 #2966

Closed

Merge pull request #2 from plotly/master

bdc94ce

catch up

ErwanMAS force-pushed the bug_trace_filltozero_order_scattergl branch from ee610d6 to 4d66f31 Compare September 12, 2018 05:13

etpinard reviewed Sep 12, 2018

View reviewed changes

src/traces/scattergl/index.js Outdated Show resolved Hide resolved

etpinard reviewed Sep 13, 2018

View reviewed changes

src/traces/scattergl/line_shape_defaults.js Outdated Show resolved Hide resolved

etpinard added the feature something new label Sep 17, 2018

etpinard reviewed Sep 17, 2018

View reviewed changes

src/traces/scattergl/index.js Outdated Show resolved Hide resolved

etpinard reviewed Sep 17, 2018

View reviewed changes

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

ErwanMAS added 13 commits September 30, 2018 11:06

Merge pull request #3 from plotly/master

e69e2d6

catchup

Fix https://codepen.io/erwanmas/pen/JaymVr

52020a9

- when undefined values with option fill tozerox or tozeroy - order of fill scattergl when overlap

fix lint issues

304083e

only one big mock for the 3 tests

05343b8

add simple shape for line in scattergl

51d6688

still missing spline from scatter

add shape to scattergl

831f508

remove some empty lines

39eb303

update of the baseline images

283b50d

we don't reuse scatter files ... because partialy impelemented

02e924c

fix lint

a49e6d0

- remove line_shape_defaults.js , moe the code into scattergl/default…

5d3a924

…s.js

fix lint issues

f670af2

- modify one test to add GL/REG version

93b61bc

- new test with more overlap with more kind of objects

ErwanMAS force-pushed the bug_trace_filltozero_order_scattergl branch from 8e87403 to 93b61bc Compare September 30, 2018 16:24

ErwanMAS added 4 commits September 30, 2018 12:34

- remove some console.log

346a32a

add baseline for the gl2d_order_error

9cf7172

modified baseline for gl2d_fill_trace_tozero_order / gl2d_scatter_fill_self_next

fix lint issues

987e6cd

last version , we display by order only .

95c602e

add somes text traces into the baseline test

etpinard added status: reviewable and removed status: in progress labels Oct 4, 2018

etpinard reviewed Oct 4, 2018

View reviewed changes

ErwanMAS added 2 commits October 5, 2018 00:28

- fix gl2d_scatter_fill_self_next ( rollback the baselines test/image…

be43294

…/baselines/gl2d_scatter_fill_self_next.png ) - add a new test with 2 draw side by side

fix lint issues

126e29c

etpinard reviewed Oct 5, 2018

View reviewed changes

fix errors display ordering

4b2b0d5

etpinard mentioned this pull request Oct 9, 2018

Scattergl improvements for: fill tozero with bad values fix, fills layering fix, add some line.shape values #3087

Merged

etpinard closed this Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fill tozero with undefined value and overlaping of fill area #2979

Fix fill tozero with undefined value and overlaping of fill area #2979

ErwanMAS commented Sep 7, 2018

etpinard commented Sep 7, 2018

etpinard Sep 12, 2018

ErwanMAS Sep 12, 2018

etpinard commented Sep 17, 2018

ErwanMAS commented Oct 1, 2018

ErwanMAS commented Oct 1, 2018

etpinard commented Oct 1, 2018

etpinard Oct 4, 2018

ErwanMAS Oct 5, 2018

etpinard Oct 5, 2018 •

edited

Loading

etpinard Oct 5, 2018

ErwanMAS Oct 6, 2018

etpinard commented Oct 9, 2018

	var fillData = [];
	if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' \|\| trace.fill === 'toself' \|\|
	(trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))
	) {
	fillData = ['_ownFill'];
	}
	if(trace._nexttrace) {
	// make the fill-to-next path now for the NEXT trace, so it shows
	// behind both lines.
	fillData.push('_nextFill');
	}

Fix fill tozero with undefined value and overlaping of fill area #2979

Fix fill tozero with undefined value and overlaping of fill area #2979

Conversation

ErwanMAS commented Sep 7, 2018

etpinard commented Sep 7, 2018

etpinard Sep 12, 2018

Choose a reason for hiding this comment

ErwanMAS Sep 12, 2018

Choose a reason for hiding this comment

etpinard commented Sep 17, 2018

ErwanMAS commented Oct 1, 2018

ErwanMAS commented Oct 1, 2018

etpinard commented Oct 1, 2018

etpinard Oct 4, 2018

Choose a reason for hiding this comment

ErwanMAS Oct 5, 2018

Choose a reason for hiding this comment

etpinard Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

etpinard Oct 5, 2018

Choose a reason for hiding this comment

ErwanMAS Oct 6, 2018

Choose a reason for hiding this comment

etpinard commented Oct 9, 2018

etpinard Oct 5, 2018 •

edited

Loading