-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: master
Are you sure you want to change the base?
Conversation
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]], |
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.
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
@dfcreative what do you think about think? |
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): Then it is somehow resized to the CSS pixel (which could represent more than one physical pixel for high DPI screens): In addition, the objects including this mesh seem to be attached an attribute pixelRatio: I don't think if we can just use that variable in mesh.js, because it is only initiated in: 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: Let me know if I misunderstand anything, thanks! FYI, these are some links might be helpful: |
@etpinard I think we have to address this issue with a test case, as it seems more complicated than just passing |
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