split the input polyline into multiple polylines when there is Nan/Nu… #49

ruijin · 2020-12-18T20:30:34Z

…ll in the input. plotly need this behavior to correctly render scatter chart.

related to this plotly issue:
plotly/plotly.js#2291 (comment)

…ll in the input. plotly need this behavior to correctly render scatter chart.

dy · 2020-12-19T00:41:49Z

index.js

+				if(!(state.count-1 in ids))
+					ids[state.count] = state.count-1   // make sure there is at least polygon
+
+				let splits = Object.keys(ids).map(e => e|0).sort(function(a, b){return a - b})


What's the purpose of map(e => e|0)? Cast to number? Better .map(Number).
Why does sort need that default comparator?

Yes, the lambda is for casting, will replace it with map(Number)
The comparator for sort is to sort by number value rather than string order.

Sorry I am not very fluent in javascript.

dy

Please provide test case in test.js.

dy · 2020-12-19T00:51:30Z

index.js

+
+				for (let i = 0; i < splits.length; i++)
+				{
+					let triangles = triangulate(pos.slice(base*2, splits[i]*2), state.hole || [])


I am not sure I follow the logic here, but placing triangulate in a loop looks suspicious.
Could you please elaborate a bit what algorithmic changes are done here?

The issue is when plotly send scatter segment to regl (which is seperated by null in plotly input), plotly close each segment and drop the nulls. When regl receive the segments, regl treat it as a single big plygon which end up drawing some extra triangles.

I submitted a PR (plotly/plotly.js#5355) to plotly to keep those nulls. However that itself is not enough since regl will replace the null with last valid point and still send a single polygon for triangulation. I don't see earcut.js supporting multiple polygon in a single input yet. So the proposed solution here is to call triangulation multiple time, each time only handle one segment (polygon).

ruijin · 2020-12-19T23:04:36Z

New submit add tests. It also handles holes now. The idea is for each polygon we create a new temporary position buffer that contains a single polygon and all the wholes (earcut.js support multiple holes). And when the vertex index comes back from earcut.js we map it back to the original position buffer.

Below are test output before and after the fix.

Before:

After:

index.js

make the index mapping of temporary clearer Co-authored-by: Mojtaba Samimi <[email protected]>

…se fillnull flag from input directly.

ruijin · 2020-12-23T08:32:49Z

regl-line2d/index.js

Lines 446 to 458 in e7ff2a4

    
           if (o.thickness != null) state.thickness = parseFloat(o.thickness) 
        
           if (o.opacity != null) state.opacity = parseFloat(o.opacity) 
        
           if (o.miterLimit != null) state.miterLimit = parseFloat(o.miterLimit) 
        
           if (o.overlay != null) { 
        
           	state.overlay = !!o.overlay 
        
           	if (i < Line2D.maxLines) { 
        
           		state.depth = 2 * (Line2D.maxLines - 1 - i % Line2D.maxLines) / Line2D.maxLines - 1.; 
        
           	} 
        
           } 
        
           if (o.join != null) state.join = o.join 
        
           if (o.hole != null) state.hole = o.hole 
        
           if (o.fill != null) state.fill = !o.fill ? null : rgba(o.fill, 'uint8') 
        
           if (o.viewport != null) state.viewport = parseRect(o.viewport)

Why here we store the input option to state? And is it true that when some field is missing in the input option, the stored value in state will be used instead? I ran into a issue during debugging notice that a path could pick up the "hole" of another path, which cause error. I don't know enough about this part of code so I did not touch it. But is this a bug?

index.js

dy · 2020-12-23T19:39:23Z

Why here we store the input option to state? And is it true that when some field is missing in the input option, the stored value in state will be used instead?

There can be multiple passes, ie. multiple lines to render, and each can have its own properties (like fill, opacity etc.), so we form state objects - each per render pass (=per shape to render). Props can be updated, yes, and not necessary all - only some props can get new value.

dy

These tests introduce WebGL warning

probably some attribute/vertex is off, worth figuring out

… 0 instead.

ruijin · 2020-12-25T01:25:54Z

when setting stroke to null, line2d will still trying to render miter join while the stroke vertex are not there. Using thickness = 0 solve (or get around) this. Not sure if this behavior is desired or not. But since it's not related to the issue of this PR, I will leave it there.

dy

LGTM

archmoj · 2020-12-26T00:50:38Z

Published [email protected] minor.

split the input polyline into multiple polylines when there is Nan/Nu…

9de15f1

…ll in the input. plotly need this behavior to correctly render scatter chart.

ruijin mentioned this pull request Dec 18, 2020

scattergl fillcolor incorrect with multiple polygons in same trace when with fill: toself plotly/plotly.js#2291

Closed

dy reviewed Dec 19, 2020

View reviewed changes

dy requested changes Dec 19, 2020

View reviewed changes

ruijin mentioned this pull request Dec 19, 2020

keep null in x,y so regl knows the end of polygon plotly/plotly.js#5355

Merged

handle hole, add 2 testes

5d54474

archmoj reviewed Dec 21, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

Update index.js

b03c773

make the index mapping of temporary clearer Co-authored-by: Mojtaba Samimi <[email protected]>

ruijin requested review from dy and archmoj December 22, 2020 02:40

ruijin added 3 commits December 21, 2020 20:04

fix the issue with ternary operator

e9d0198

add option to control the behavior of fill with Null/NaN

75188f7

use findIndex polyfill for better compatability with older browser. U…

8a635df

…se fillnull flag from input directly.

dy reviewed Dec 23, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

switch to a lighter polyfill for findIndex

73543b3

ruijin requested a review from dy December 24, 2020 04:31

dy requested changes Dec 24, 2020

View reviewed changes

dy and others added 2 commits December 24, 2020 14:07

Cleanup

26a8ece

Setting stroke to null throw off the join rendering. Set thickness to…

d66c942

… 0 instead.

ruijin requested a review from dy December 25, 2020 01:26

dy approved these changes Dec 25, 2020

View reviewed changes

archmoj merged commit dec9c08 into gl-vis:master Dec 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split the input polyline into multiple polylines when there is Nan/Nu… #49

split the input polyline into multiple polylines when there is Nan/Nu… #49

ruijin commented Dec 18, 2020

dy Dec 19, 2020 •

edited

Loading

ruijin Dec 19, 2020

dy left a comment

dy Dec 19, 2020

ruijin Dec 19, 2020 •

edited

Loading

ruijin commented Dec 19, 2020 •

edited

Loading

ruijin commented Dec 23, 2020

dy commented Dec 23, 2020 •

edited

Loading

dy left a comment

ruijin commented Dec 25, 2020

dy left a comment

archmoj commented Dec 26, 2020

split the input polyline into multiple polylines when there is Nan/Nu… #49

split the input polyline into multiple polylines when there is Nan/Nu… #49

Conversation

ruijin commented Dec 18, 2020

dy Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

ruijin Dec 19, 2020

Choose a reason for hiding this comment

dy left a comment

Choose a reason for hiding this comment

dy Dec 19, 2020

Choose a reason for hiding this comment

ruijin Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

ruijin commented Dec 19, 2020 • edited Loading

ruijin commented Dec 23, 2020

dy commented Dec 23, 2020 • edited Loading

dy left a comment

Choose a reason for hiding this comment

ruijin commented Dec 25, 2020

dy left a comment

Choose a reason for hiding this comment

archmoj commented Dec 26, 2020

dy Dec 19, 2020 •

edited

Loading

ruijin Dec 19, 2020 •

edited

Loading

ruijin commented Dec 19, 2020 •

edited

Loading

dy commented Dec 23, 2020 •

edited

Loading