Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scattergl fillcolor incorrect with multiple polygons in same trace when with `fill: toself` #2291

Closed
cpsievert opened this issue Jan 24, 2018 · 22 comments
Labels
Milestone

Comments

@cpsievert
Copy link

@cpsievert cpsievert commented Jan 24, 2018

https://codepen.io/cpsievert/pen/ZvNqjQ

Note that fillcolor renders correctly when type: scatter.

@jackparmer
Copy link
Contributor

@jackparmer jackparmer commented Jan 24, 2018

cc @Kully - this is going to be an issue with the Python county choropleth as well. Probably watch this issue and hold off on the WebGL approach until this is fixed:

image

@Kully
Copy link
Contributor

@Kully Kully commented Jan 24, 2018

I actually found that toself fill didn't work with just one polygon (ie list of coordinates defining a contained polygon)

But I only checked in the Python API

@jackparmer
Copy link
Contributor

@jackparmer jackparmer commented Jan 26, 2018

Here is what I get for a full US county map for SVG vs WebGL using toself:

image

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Mar 8, 2018

related: #2450

@Kully
Copy link
Contributor

@Kully Kully commented May 28, 2018

Another example of the USA choropleth from here: https://plot.ly/python/county-choropleth/#the-entire-usa This is switching scatter -> scattergl in the figurefactory.

screen shot 2018-05-28 at 12 07 32 pm

@jackparmer
Copy link
Contributor

@jackparmer jackparmer commented May 28, 2018

⬆️ Doesn't look like the recent scattergl fill changes fixed things for this particular use case

@etpinard
Copy link
Contributor

@etpinard etpinard commented May 28, 2018

Doesn't look like the recent scattergl fill changes fixed things for this particular use case

There haven't been any scattergl fill changes since the regl push back January.

@jackparmer
Copy link
Contributor

@jackparmer jackparmer commented May 28, 2018

Oh, I thought @dy had done some work on regl-line2d fills after January. I guess I'm misremembering.

@etpinard
Copy link
Contributor

@etpinard etpinard commented May 28, 2018

He's done some work. But none of which is ready to get merged at this stage.

@dy
Copy link
Contributor

@dy dy commented May 28, 2018

It is coming, after regl-text. Pls stay tuned)

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Jul 26, 2018

A nice pared-down reproduction from @llnns in #2845:


Using scattergl with toself fill option results in a "ghost"/wrong/extra fill among the first points of each object. A minimal example creating three rectangles:

https://codepen.io/anon/pen/NBvbbv

Results in:

newplot

Also, using the same data with scatter type has the expected output:

newplot 1

@llnns
Copy link

@llnns llnns commented Jul 26, 2018

Hey, maybe another hint here:
Using https://codepen.io/anon/pen/NBvbbv as an example.

It seems that the extra triangle comes from the triangulation function earcut. The block at

else if(trace.fill === 'toself' || trace.fill === 'tonext') {
pos = [];
last = 0;
for(j = 0; j < srcPos.length; j += 2) {
if(isNaN(srcPos[j]) || isNaN(srcPos[j + 1])) {
pos = pos.concat(srcPos.slice(last, j));
pos.push(srcPos[last], srcPos[last + 1]);
last = j + 2;
}
}
pos = pos.concat(srcPos.slice(last));
if(last) {
pos.push(srcPos[last], srcPos[last + 1]);
}
}
is generating the follow pos array:

pos = [
  0,
  9,
  0,
  10,
  1,
  10,
  1,
  9,
  0,
  9,
  0,
  9,
  1,
  6,
  1,
  8,
  2,
  8,
  2,
  6,
  1,
  6,
  1,
  6,
  2,
  1,
  2,
  2,
  3,
  2,
  3,
  1,
  2,
  1,
  2,
  1
]

That it is been pass to earcut function as earcut(pos, []) (I think at: https://github.com/a-vis/regl-line2d/blob/d4e34d3d38b20fce5bc0398e3cf30181d1f506a0/index.js#L519 ) resulting in the follow triangles indices array:

triangles = [
  15,
  14,
  13,
  9,
  8,
  7,
  3,
  2,
  1,
  4,
  3,
  1,
  15,
  13,
  12,
  6,
  0,
  12,
  7,
  10,
  9
]

Notice that the result has 21 indices, 7 triangles, one more than the 6 needed for the 3 rectangles example. Also, I am not sure if it is intended, but in the pos array the last position of each rectangle is duplicated.

@etpinard
Copy link
Contributor

@etpinard etpinard commented Sep 19, 2018

A very nice example from @ErwanMAS in #3025


Please look .

https://codepen.io/erwanmas/pen/dqQrbR

The issue is generated by https://github.com/mapbox/earcut in https://github.com/a-vis/regl-line2d . AND earcut doesn't guarantee correctness

https://github.com/mapbox/earcut/blob/master/README.md

a simple console.log here https://github.com/a-vis/regl-line2d/blob/b9d56d51e1598c4ee7a9a5e0a733e920daf63c2d/index.js#L520 will output only 1 triangle , instead of 2 triangles

@ruijin
Copy link
Contributor

@ruijin ruijin commented Apr 25, 2020

I was trying to use plotly's scattergl to rendering thousands of boxes and hit this old issue. It seems like nobody is working on it so I would like to give it a try.

I think this is the cause of the issue:

  1. for scatter trace, we use null in x or y to "break" the plotline, as @llnns showed in his/her nice example.

  2. when plotly sees the nulls it close the polyline and drop the null.

    else if(trace.fill === 'toself' || trace.fill === 'tonext') {
    pos = [];
    last = 0;
    for(j = 0; j < srcPos.length; j += 2) {
    if(isNaN(srcPos[j]) || isNaN(srcPos[j + 1])) {
    pos = pos.concat(srcPos.slice(last, j));
    pos.push(srcPos[last], srcPos[last + 1]);
    last = j + 2;
    }
    }
    pos = pos.concat(srcPos.slice(last));
    if(last) {
    pos.push(srcPos[last], srcPos[last + 1]);
    }
    }

  3. regl.Line2D get the polyline and use earcut algorithm to triangulate the whole ply line as single polygon.

https://github.com/gl-vis/regl-line2d/blob/0fdebe97ced51782e5f1e7b1c5c532a6c86d2140/index.js#L504-L519

As a result of above, we end up with a strange shape polygon, use @llnns's example again:

image

I don't think this is a bug in https://github.com/mapbox/earcut , since it does what regl ask it to do: triangulate a single polygon.

The solution I am proposing is:

  1. plotly keep the nulls when sending ployline to regl.Line2D
  2. regl recognizes the nulls, and break the ployline into separate polygons and triangulate each of them separately.

I have a prototype working locally, at least for my usecase (fill=toself). But this fix requires a pull request to both plotly and regl.Line2D, and I am not expert in either javascript nor github, so I would really appreciate some guidance here.

@rlele5
Copy link

@rlele5 rlele5 commented Jun 11, 2020

Hi all, still having this issue. Are there any plans to merge any of the proposed solutions into main plotly? This single bug fix could potentially fix so many of my plotting performance issues, so I'm eagerly looking forward to it! Hopefully it can be prioritized.

Another example posted here if needed: https://community.plotly.com/t/feature-request-for-gantt-gl-like-scatter-gl/37925/4

Tag: @archmoj, @etpinard

@jackparmer
Copy link
Contributor

@jackparmer jackparmer commented Sep 10, 2020

This issue has been tagged with NEEDS SPON$OR

A community PR for this feature would certainly be welcome, but our experience is deeper features like this are difficult to complete without the Plotly maintainers leading the effort.

Sponsorship range: $15k-$20k

What Sponsorship includes:

  • Completion of this feature to the Sponsor's satisfaction, in a manner coherent with the rest of the Plotly.js library and API
  • Tests for this feature
  • Long-term support (continued support of this feature in the latest version of Plotly.js)
  • Documentation at plotly.com/javascript
  • Possibility of integrating this feature with Plotly Graphing Libraries (Python, R, F#, Julia, MATLAB, etc)
  • Possibility of integrating this feature with Dash
  • Feature announcement on community.plotly.com with shout out to Sponsor (or can remain anonymous)
  • Gratification of advancing the world's most downloaded, interactive scientific graphing libraries (>50M downloads across supported languages)

Please include the link to this issue when contacting us to discuss.

@rlele5
Copy link

@rlele5 rlele5 commented Sep 12, 2020

Thanks for the update :). Now another feature request for @github, if you're listening - we should have Gofundmes attached to issues! I'd gladly commit a few hundred to this issue if it could be done in a safe and organized way

@avsdev-cw
Copy link

@avsdev-cw avsdev-cw commented Nov 9, 2020

I would also like to see this fixed, so +1 for that.

However that I feel that sponsership range is pretty excessive considering a bunch of people have already dug out what the root cause appears to be...

@ruijin
Copy link
Contributor

@ruijin ruijin commented Dec 18, 2020

Created two PR to plotlyjs and regl

#5355
gl-vis/regl-line2d#49

I am happy to help with any further testing, as long as this got fixed.

@rlele5
Copy link

@rlele5 rlele5 commented Dec 31, 2020

@ruijin , thanks for making the pull requests! I'm excited to try them out!

@rlele5
Copy link

@rlele5 rlele5 commented Jan 14, 2021

Hi @ruijin , I tried manually updating the /Library/Frameworks/Python.framework/Versions/3.7/share/jupyter/lab/staging/node_modules/plotly.js/src/traces/scattergl and /Library/Frameworks/Python.framework/Versions/3.7/share/jupyter/lab/staging/node_modules/regl-line2d/index.js code and then I restarted my jupyter notebook to see if it worked. However, I still saw the same issue. I'm pretty sure the issue is on my end, but do you know how I can get my plotly.py library to "uptake" the changes that I manually made to plotly.js? I've tried deleting plotly.py's __pycache__ and restarting/rerunning to no avail.

@archmoj archmoj added this to the NEXT milestone Jan 14, 2021
@archmoj
Copy link
Collaborator

@archmoj archmoj commented Jan 14, 2021

Fixed via #5355.

@archmoj archmoj closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.