CARTAvis / carta-frontend

Source code repository for the frontend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
20 stars 10 forks source link

blurry catalog overlay on Retina display #889

Closed kswang1029 closed 3 years ago

kswang1029 commented 4 years ago

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

veggiesaurus commented 4 years ago

It looks like plotly is not taking into account the devicePixelRatio: https://github.com/plotly/plotly.js/issues/3246#issuecomment-439175019

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 commented 4 years ago

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 commented 4 years ago

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 :thinking:

veggiesaurus commented 4 years ago

@panchyo0 are you investigating this?

panchyo0 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

fixed with PR 1211