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

blurry catalog overlay on Retina display #889

Closed
kswang1029 opened this issue Apr 20, 2020 · 10 comments
Closed

blurry catalog overlay on Retina display #889

kswang1029 opened this issue Apr 20, 2020 · 10 comments
Assignees
Labels
screen dpi For issues relating to devicePixelRatio

Comments

@kswang1029
Copy link
Collaborator

It appears that the rendered catalog overlay is blurry on Retina display. On a non-retina display, the catalog overlay is sharp.

@kswang1029 kswang1029 added the screen dpi For issues relating to devicePixelRatio label Apr 20, 2020
@veggiesaurus
Copy link
Collaborator

veggiesaurus commented Apr 22, 2020

It looks like plotly is not taking into account the devicePixelRatio: plotly/plotly.js#3246 (comment)

I tried to solve the issue by doubling the desired size and then transforming using CSS. It looked great, but it didn't seem to work nicely with the single point selection stuff. or with zooming. @panchyo0 if you want to give it a try, here's a patch of what I tried:

Index: src/components/ImageView/CatalogView/CatalogViewComponent.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/ImageView/CatalogView/CatalogViewComponent.tsx	(revision Local Version)
+++ src/components/ImageView/CatalogView/CatalogViewComponent.tsx	(revision Shelved Version)
@@ -40,7 +40,7 @@
             data.marker = {
                 symbol: catalog.shape, 
                 color: catalog.color,
-                size: catalog.size,
+                size: catalog.size * devicePixelRatio,
                 line: {
                     width: 1.5
                 }
@@ -100,8 +100,8 @@
 
     render() {
         const frame = this.props.appStore.activeFrame;
-        const width = frame ? frame.renderWidth || 1 : 1;
-        const height = frame ? frame.renderHeight || 1 : 1;
+        const width = frame ? frame.renderWidth * devicePixelRatio || 1 : 1;
+        const height = frame ? frame.renderHeight * devicePixelRatio || 1 : 1;
         const padding = this.props.overlaySettings.padding;
         let className = "catalog-div";
         if (this.props.docked) {
@@ -160,6 +160,7 @@
                     layout={layout}
                     config={config}
                     onClick={this.onClick}
+                    style={{transform: "scale(0.5)", transformOrigin: "top left"}}
                 />
             </div>
         );

@panchyo0
Copy link
Contributor

Using transform: scale() will cause this bug.
Example to show the offset with css scale() https://codepen.io/anon/pen/ZXdYdb.
Since we are using the default onClick() for source selection, this will effect us.

@veggiesaurus
Copy link
Collaborator

Since we are using the default onClick() for source selection, this will effect us.

ok, so we should be able to translate those points based on the devicePixelRatio, right? Not sure if there's a better way of handling HiDPi plots, but I'd have thought that Plotly.js would support HiDPi out of the box 🤔

@veggiesaurus
Copy link
Collaborator

@panchyo0 are you investigating this?

@panchyo0
Copy link
Contributor

panchyo0 commented Jul 9, 2020

@veggiesaurus Using style={{zoom: "1/devicePixelRatio"}} or set style={{transform: "scale(1/devicePixelRatio)", transformOrigin: "top left"}} at gl canvas will fix the zoom bug.
since we translate the source x and y position according devicePixelRatio and just visually zoom them back to the origin position. The clientX and clientY from the mouse event need to be translated too, in order to get the right position for plotly.js to find the selected source. Haven't fond a way to translate the clientX and clientY for plotly. Should we write our own method for search?
The HiDPi issue for plotly.js only happened on scattergl type.

@veggiesaurus
Copy link
Collaborator

Is there an issue for this mentioned anywhere on the plotly repo? It seems like this would be a common issue 🤔

The reason it only happens with scattergl is because the WebGL canvas itself is not resized properly. Perhaps we can manually force it to resize 🤔

@panchyo0
Copy link
Contributor

Is there an issue for this mentioned anywhere on the plotly repo? It seems like this would be a common issue
they mentioned on the transform: scale()
Perhaps we can manually force it to resize
I try to resize through css, onClick() still can not find the right position.

@panchyo0
Copy link
Contributor

panchyo0 commented Dec 9, 2020

With plotlyjs 1.58.0, we can apply transform to fix this bug. @kswang1029 could you please check http://carta.asiaa.sinica.edu.tw/frontend/0b759da/?socketUrl=wss://carta.asiaa.sinica.edu.tw/socketdev

@kswang1029
Copy link
Collaborator Author

kswang1029 commented Dec 9, 2020

@panchyo0 yes now with a Retina display, image overlay is sharp.

I noticed a performance difference when plotting 20000 symbols with Chrome, Firefox, and Safari. They perform like Chrome (smooth) > Firefox (a bit leggy) > Safari (quite leggy)

@panchyo0 @veggiesaurus could this be the issue we encountered before that Safari updates cursor state at 240 Hz?

UPDATE: with Safari, the current dev branch has the same issue but less critical. With 0b759da it becomes worse.

@panchyo0
Copy link
Contributor

panchyo0 commented Feb 6, 2021

fixed with PR 1211

@panchyo0 panchyo0 closed this as completed Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
screen dpi For issues relating to devicePixelRatio
Projects
None yet
Development

No branches or pull requests

3 participants