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

Fix for scattergl reversed lines bug #3078

Merged
merged 9 commits into from Oct 24, 2018
Merged

Fix for scattergl reversed lines bug #3078

merged 9 commits into from Oct 24, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 4, 2018

Fixes #2904

Bug fix for 2D line plots.
Negative scales are handled in the webgl vertex shader so that the line plots would be displayed in layouts with reversed ranges.
@etpinard
@alexcjohnson

@etpinard
Copy link
Contributor

etpinard commented Oct 5, 2018

Thanks @archmoj !

Pro tip: make the PR title descriptive, so that we have an idea of what the PR is about just by looking at the title. No one remember issue numbers. Thanks again!

@etpinard etpinard changed the title issue #2904 Fix for scattergl reversed lines bug Oct 5, 2018
package.json Outdated
@@ -102,7 +102,7 @@
"polybooljs": "^1.2.0",
"regl": "^1.3.7",
"regl-error2d": "^2.0.5",
"regl-line2d": "^3.0.11",
"regl-line2d": "git://github.com/archmoj/regl-line2d.git#c5856f8d71f246532f11cb48e20aad30fa773652",
Copy link
Contributor

Choose a reason for hiding this comment

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

gl-vis/regl-line2d@master...archmoj:rev-bounds2d

@dy would you mind taking a 👀 at this patch?

@etpinard
Copy link
Contributor

etpinard commented Oct 5, 2018

@archmoj I'm curious. What made you decide to patch the regl-line2d?

From https://codepen.io/anon/pen/EpJObQ, swapping mode: 'lines' for mode: 'lines+markers' makes things work leading me to believe that we "could" fix this in plotly.js' scattergl/index.js.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 5, 2018

@etpinard
When using only lines (without markers, etc.) using reversed ranges used to create a horizontal or vertical line rather than actual graph.
Please try this:

Plotly.newPlot(gd, [{ "type": "scattergl", "mode": "lines", "x": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], "y": [0, 1, 0, 7, 7, 0, 2, 4, 3, 0], "line": { "width": 10, "dash": "solid" } }], { xaxis:{ range: [-1, 10] }, yaxis:{ range: [10, -1] }}) ;

@etpinard
Copy link
Contributor

etpinard commented Oct 5, 2018

Yeah I know, but

Plotly.newPlot(gd, [{ "type": "scattergl", "mode": "markers+lines", "x": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], "y": [0, 1, 0, 7, 7, 0, 2, 4, 3, 0], "line": { "width": 10, "dash": "solid" } }], { xaxis:{ range: [-1, 10] }, yaxis:{ range: [10, -1] }}) ;

gives the correct result:

image

with the current master regl-line2d shader.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 5, 2018

I see that the point of a patch in javascript file.
However with in the shader we defined and used 'scale' both as a local variable and uniform!

  1. uniform vec2 scale, translate; // global
  2. vec2 scale = max(scale, MIN_DIFF); // local
    And I think in the example image provided above the corners may not be properly rendered?

@archmoj
Copy link
Contributor Author

archmoj commented Oct 8, 2018

To compare with the above image here is an example of applying only lines with round corners (using new regl-line2d shader).
newplot 22 - copy

Plotly.newPlot(gd, [{ "type": "scattergl", "mode": "lines", "x": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], "y": [0, 1, 0, 7, 7, 0, 2, 4, 3, 0], "line": { "width": 10, "dash": "solid" } }], { xaxis:{ range: [-1, 10] }, yaxis:{ range: [10, -1] }}) ;

@etpinard
Copy link
Contributor

etpinard commented Oct 9, 2018

However with in the shader we defined and used 'scale' both as a local variable and uniform!

To compare with the above image here is an example of applying only lines with round corners (using new regl-line2d shader).

Thanks for the explanation. This makes it very clear. Don't be shy to write down comments on "important" lines on your github PRs to add a little bit of backstory to your work. This makes the review much easier (and faster).


Now, I think the next step would be the make a PR to https://github.com/a-vis/regl-line2d and ping @dy

@archmoj
Copy link
Contributor Author

archmoj commented Oct 14, 2018

Not being listed in the contributors of regl I was not able to push my branch for the fix to a-vis/regl-line2d (rather than my fork) so that it could be on a-vis. A similar permission may be required for regl npm packages.
@dy
@alexcjohnson
@bpostlethwaite

@alexcjohnson
Copy link
Contributor

Looks good to me! 💃

@archmoj archmoj merged commit 94c9d98 into master Oct 24, 2018
@archmoj archmoj deleted the rev-bounds2d branch October 24, 2018 17:27
@etpinard
Copy link
Contributor

Lovely fix 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scattergl reversed lines bug
3 participants