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 mouse event on high resolution screens #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kapythone
Copy link

It doesn't handle cases for high resolution screens such as Retina,

You can test it by running tests in plotly.js:

npm run test-jasmine -- gl_plot_interact

mesh.js Outdated
@@ -764,7 +764,7 @@ proto.pick = function(pickData) {

var data = closestPoint(
simplex,
[pickData.coord[0], this._resolution[1]-pickData.coord[1]],
[window.devicePixelRatio * pickData.coord[0], this._resolution[1] - window.devicePixelRatio * pickData.coord[1]],
Copy link
Member

@rreusser rreusser Apr 24, 2017

Choose a reason for hiding this comment

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

I don't know the full structure of these libs, but is it possible or desirable to get the pixel ratio from the scene instead of the window? Seems it's possible to set a device pixel ratio that might differ from the window device pixel ratio: https://github.com/gl-vis/gl-plot3d/blob/e573384c1302f8c4358e85faf5293c74447dc25f/scene.js#L150

@etpinard
Copy link
Member

@dfcreative what do you think about think?

@kapythone
Copy link
Author

I checked the relevant codes again and found it's more complicated than I think.

First, pickBuffer was initiated with ViewShape (whose size is in the unit of physical pixels):
https://github.com/gl-vis/gl-plot3d/blob/master/scene.js#L241

Then it is somehow resized to the CSS pixel (which could represent more than one physical pixel for high DPI screens):
https://github.com/gl-vis/gl-plot3d/blob/master/scene.js#L442
I don't quite understand the logic here, since the buffer is still initiated with previous physical pixels.

In addition, the objects including this mesh seem to be attached an attribute pixelRatio:
https://github.com/gl-vis/gl-plot3d/blob/master/scene.js#L491

I don't think if we can just use that variable in mesh.js, because it is only initiated in:
https://github.com/gl-vis/gl-plot3d/blob/master/scene.js#L491

As far as I know, we should convert the position of x and y, not using another pickShape, the commits related to pickShape such as this one are not necessary:

gl-vis/gl-plot3d@b5c06ad

Let me know if I misunderstand anything, thanks!

FYI, these are some links might be helpful:

w3c/uievents#40

https://www.khronos.org/webgl/wiki/HandlingHighDPI

@dy
Copy link
Member

dy commented Apr 24, 2017

@etpinard I think we have to address this issue with a test case, as it seems more complicated than just passing pixelRatio from options.

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

Successfully merging this pull request may close these issues.

4 participants