gl-vis / regl-line2d

:part_alternation_mark: Draw 2d polyline with regl
https://gl-vis.github.io/regl-line2d
MIT License
59 stars 15 forks source link

`.draw()` doesn't skip over ids with undefined options #35

Closed etpinard closed 6 years ago

etpinard commented 6 years ago

@dy

let regl = require('regl')({extensions: 'angle_instanced_arrays'})
let line2d = require('./')(regl)

line2d.update([
  { thickness: 4, points: [0,0, 1,1, 1,0], close: true, color: 'red' },
  undefined,
  { thickness: 4, points: [0,1, 1,1, 0,0], close: true, color: 'blue' }
])
line2d.draw(0)
line2d.draw(2)

only draws the red triangle.


Example in plotly.js https://codepen.io/etpinard/pen/bKQjMe

ErwanMAS commented 6 years ago

First from here we assume that at index i of the array passes , we found element with id i https://github.com/a-vis/regl-line2d/blob/0c25f299abab2e714b7ce68fd585df6d10e533cb/index.js#L407-L408)

But here we remove empty cell from the array passes , so at index i i dont have any more element with id i https://github.com/a-vis/regl-line2d/blob/0c25f299abab2e714b7ce68fd585df6d10e533cb/index.js#L692

so when we call draw with a number https://github.com/a-vis/regl-line2d/blob/0c25f299abab2e714b7ce68fd585df6d10e533cb/index.js#L333

we can draw the wrong element

etpinard commented 6 years ago

@ErwanMAS thanks for the investigation.

I haven't looked at this bug in a while now, but I suspect the best what to fix it would be to implement whether regl-scatter2d does for skipping empty passes.

etpinard commented 6 years ago

@dy could you help us out?

dy commented 6 years ago

3.0.11 should fix that

ErwanMAS commented 6 years ago

Hey , The patch solve only one case but not other more complex case .

can you test against https://codepen.io/erwanmas/pen/NLwXab ?

etpinard commented 6 years ago

@ErwanMAS using regl-line2d@3.0.11, I'm getting:

image

vs

image

where the fill pattern in the red trace is wrong, but the rest appears right.

etpinard commented 6 years ago

PR for plotly.js -> https://github.com/plotly/plotly.js/pull/2990 with corrected baselines.

ErwanMAS commented 6 years ago

@dy @etpinard i am still thinking , this is not the right solution .

I applied only the patch and tested and this does not work
https://codepen.io/erwanmas/pen/JapLZb

etpinard commented 6 years ago

Thanks @ErwanMAS !

I confirm your observations on the https://github.com/plotly/plotly.js/pull/2990 branch. The problem seems to only be apparent on "fills" traces. Dima's patch seems like a step in the right direction.

@ErwanMAS can you replicate the problem outside of plotly.js with just regl-line2d?

For future reference, here's a more simple codepen https://codepen.io/etpinard/pen/WgzrVw?editors=1010 (with a legend and the default trace colors making it easier to tell which trace is which).

etpinard commented 6 years ago

@ErwanMAS the problem was in plotly.js.

I was able to fix it in https://github.com/plotly/plotly.js/pull/2990/commits/045a162ee55b059ad37e89e339e0213dd3897d50

Thanks for your help!

ErwanMAS commented 6 years ago

How i can run test outside of plotly.js ? @etpinard

dy commented 6 years ago

I guess this issue is fixed via https://github.com/plotly/plotly.js/pull/2990