-
Notifications
You must be signed in to change notification settings - Fork 5
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
Maintaining marker size with various pixelRatio values #20
Merged
Merged
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0fcb2a5
do not scale markers and circles using pixelRatio
archmoj 6a8f357
update bundle
archmoj b5f0cf5
Merge branch 'master' into fix3246-glPixelRatio-change-marker-size
archmoj 1899089
update bundle
archmoj 8b781ac
adjust tests to maintain images and perform similar to previous use o…
archmoj 9c0933a
added constPointSize flag
archmoj df5b691
Merge remote-tracking branch 'origin/master' into fix3246-glPixelRati…
archmoj fd1db24
remove package-lock
archmoj 758b06f
update bundle
archmoj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not fixing pixel ratio from uniforms side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dy for looking at this.
It seems to be used in one other place in the shader:
regl-scatter2d/marker-vert.glsl
Line 42 in 6a8f357
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But fragPointSize is different from gl_PointSize. Can you possibly add a test-case for the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pixelRatio
could be set as a constant of 2 here:regl-scatter2d/scatter.js
Line 84 in 091a1a7
pixelRatio
values are possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be done as a separate option if you want to make some default value. Here https://github.com/gl-vis/regl-scatter2d/blob/master/scatter.js#L199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. Why not avoiding passing
plotGlPixelRatio
aspixelRatio
at all?The cases mentioned in the codepen make sure the point size is consisted in devices with various
pixelRatio
. If you want to pass "fake" pixelRatio - the point size will be increased, that's what expected. Am I getting it wrong?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Plotly.js
plotGlPixelRatio
is the same aspixelRatio
. Here is another codepen example comparingscatter
,scattergl
&scatter3d
with different pixelRatio values lines and markers. The only module which treat pixelRatio differently seems to be this module. When passing different pixelRatio values we don't expect a change in point sizes but slightly higher render quality.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered that on the devices with another default pixelRatio? The point size is likely smaller there, other gl- components don’t consider that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The codepens on different devices appear to be rendered OK with this fix.
@etpinard what's your thoughts here.
Should we consider another fix something like this? https://github.com/plotly/plotly.js/compare/markersize-second-approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your hard work @archmoj - your patch appears to make
scattergl
consistent with how markers are drawn inscatter3d
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 toregl-scatter2d
that we would turn on upon instantiating in plotly.js