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

keep null in x,y so regl knows the end of polygon #5355

Merged
merged 7 commits into from
Dec 26, 2020

Conversation

ruijin
Copy link
Contributor

@ruijin ruijin commented Dec 18, 2020

Fixes #2291

@archmoj
Copy link
Contributor

archmoj commented Dec 18, 2020

Thanks very much for the PR! 🥇
Could you please add a mock and baseline to our image tests similar to this codepen?
#2291 (comment)

Here is more info https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md#image-pixel-comparison-tests

@archmoj
Copy link
Contributor

archmoj commented Dec 18, 2020

Unfortunately these changes do not appear to fix the problem on this codepen.
@ruijin could you guide me to a case (data/layout) where your patch fixed the problem, please?

@ruijin
Copy link
Contributor Author

ruijin commented Dec 19, 2020

I will try to provide a case. But just in case, did you test it together with the regl-line2d PR I submitted together? gl-vis/regl-line2d#49

@ruijin
Copy link
Contributor Author

ruijin commented Dec 19, 2020

I modify the existing gl2d_scatter_fill_self_next test to add test with >2 segment, which exposes the issue. Below is the screen shot on my local run before and after the fix. You will need the PR in regl (gl-vis/regl-line2d#49) to see it.

Before
plotly_pre_fix
After
plotly_post_fix

@archmoj
Copy link
Contributor

archmoj commented Dec 21, 2020

I will try to provide a case. But just in case, did you test it together with the regl-line2d PR I submitted together? gl-vis/regl-line2d#49

Please edit the link for regl-line2d in package.json
So instead of

"regl-line2d": "^3.0.18",

you should provide the last commit hash and address to your fork of regl-line2d which is

"regl-line2d": "git://github.com/ruijin/regl-line2d.git#5d544746bbbcf09223281a0da5549a485782ffe1",

Then run npm install so that package-lock.json is updated.
Finally please commit the changes to package.json and package-lock.json files and push.

@ruijin
Copy link
Contributor Author

ruijin commented Dec 22, 2020

The CI kept failing at random place, it does seem like triggered by the code I touched?

@archmoj
Copy link
Contributor

archmoj commented Dec 22, 2020

The CI kept failing at random place, it does seem like triggered by the code I touched?

I rerun the tests. The jasmine ones now pass. But there is a problem catch by image test.
Could you investigate why gl2d_scatter_fill_self_next does not render when making the baseline?

@ruijin
Copy link
Contributor Author

ruijin commented Dec 23, 2020

The issue of baseline is that I used Array.findIndex in the new code, and the image test server used by the test framework is a bit old and does not support that. So I use a polyfill instead. It should be fixed now.

@ruijin ruijin force-pushed the scattergl_fill_fix branch from aefb748 to 4c19b40 Compare December 25, 2020 17:14
package.json Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ruijin ruijin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update regl-line2d to latest official version

@archmoj
Copy link
Contributor

archmoj commented Dec 26, 2020

Nicely done.
💃

@archmoj archmoj merged commit 4ce0c8b into plotly:master Dec 26, 2020
@archmoj archmoj added this to the 1.59.0 milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scattergl fillcolor incorrect with multiple polygons in same trace when with fill: toself
2 participants