search

gl-vis / regl-scatter2d

Fast and precise scatter plot
https://gl-vis.github.io/regl-scatter2d
MIT License
20 stars 5 forks source link

Maintaining marker size with various pixelRatio values #20

Closed archmoj closed 4 years ago

archmoj commented 5 years ago

Added new option constPointSize to help maintain marker sizes with different pixelRatio values.

Fix for issue https://github.com/plotly/plotly.js/issues/3246. Kindly visit PR https://github.com/plotly/plotly.js/pull/3637 for more information. @dy cc: @etpinard

archmoj commented 5 years ago

@dy @etpinard OK. The tests are tweaked with half sizes and passed noting that we should maintain sizes identical to (the old use of) pixelRatio=2 in plotly.js baselines. If all is good we may publish a minor and include these changes in the next plotly.js minor.

archmoj commented 5 years ago

@dy Possibly any updates on this? If you still think that this is not the right fix, please let me know. Maybe we could find another way to tweak the API on the plotly.js side? Thank you in advance.

dy commented 5 years ago

@archmoj I did not mean to delay, sorry. Yes, I think that now pixelRatio is handled correctly - it scales points up depending on the device or passed pixelRatio. If we remove that support, the regl-scatter2d will render points with 1:1 pixels binding, which makes pixelRatio option useless. If you want to fix pixelRatio to some value, you can do that externally, that option is exactly for that.

archmoj commented 5 years ago

@dy Thank you very much for the follow ups. In fact we prefer not to pass a constant value there. Here is another codepen comparing pixelRatio values equal to 1 and 2 that uses current regl-line2d & regl-scatter2d modules. The line widths are identical but the marker sizes are not.

dy commented 5 years ago

Yep, the problem is on the regl-line2d side. It just doesn't account for pixelRatio. regl-scatter2d works as intended.

archmoj commented 5 years ago

Thanks for your hard work @archmoj - your patch appears to make scattergl consistent with how markers are drawn in scatter3d so

As this repo remains in @dy 's hands, we could perhaps make this new/consistent pixelRatio behaviour an opt-in e.g. by adding a boolean flag to regl-scatter2d that we would turn on upon instantiating in plotly.js

Great idea @etpinard. Applied in https://github.com/gl-vis/regl-scatter2d/pull/20/commits/9c0933a1df175f0bb6ba39a5e71e351e67161f2d.

@dy this should be good to go as tested on plotly side as well as here without any changes to the tests. Would you mind reviewing it again? Thanks in advance.

dy commented 5 years ago

Gentlemen, I can't understand - why passing plotly's pixelRatio at all, if you need that constant? Just initialize regl-scatter2d with {pixelRatio: 2} from any trace and we're good to go, it will behave the same way as the other gl components. It is also shorter than {pixelRatio: plotlyPixelRatio, constantPixelRatio: true}.

etpinard commented 5 years ago

, I can't understand - why passing plotly's pixelRatio at all, if you need that constant?

We offer that option to users on static exports (example) - which can make a appreciable difference in the quality of the exported images.

dy commented 5 years ago

@etpinard that's great option, but no need to pass it to regl-scatter, pass {pixelRatio: constant} instead and that's it. Eg. here https://github.com/plotly/plotly.js/blob/225521271f2eb7f5803713a28d8a7e0ef16ccc57/src/traces/scattergl/index.js#L116 just add an extra-line

// fix pixel ratio to accommodate other gl-components api
opts.marker.pixelRatio = 2.
archmoj commented 5 years ago

@dy Thanks for the suggestion. Unfortunately it does not seem to help fix the issue : ( Here is a codepen based on the fix you suggested. Please notice how the markers are getting bigger with greater pixelRatio values.