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

Cesium looks aliased on MacBook Pro Retina display #3249

Closed
pjcozzi opened this issue Nov 26, 2015 · 13 comments · Fixed by #3288
Closed

Cesium looks aliased on MacBook Pro Retina display #3249

pjcozzi opened this issue Nov 26, 2015 · 13 comments · Fixed by #3288

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 26, 2015

@mramato perhaps this is due to #3233?

master:
image

1.15:
image

@mramato
Copy link
Contributor

mramato commented Nov 26, 2015

What is the value of window.devicePixelRatio on that machine? If you set viewer.resolutionScale = window.devicePixelRatio, then you'll get the old behavior.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 26, 2015

window.devicePixelRatio is 2.

If you set viewer.resolutionScale = window.devicePixelRatio, then you'll get the old behavior.

Are you suggesting this as a workaround? These laptops are popular enough that Cesium needs to looks crisp by default.

@mramato
Copy link
Contributor

mramato commented Nov 27, 2015

Are you suggesting this as a workaround? These laptops are popular enough that Cesium needs to looks crisp by default.

I totally agree. The issue here is that the scene is being rendered at a lower-resolution (as requested by the browser) and then resized with smoothing, but simply going back to the old behavior isn't a good solution for 3 key reasons:

  1. A blank globe should be capable of running at 60fps. If it looks crisp and but runs at 20fps, that's just as bad. Macbooks are popular, but Macbooks with good video cards aren't. Most models will be rendering at 2560x1600 or 2880x1800 on an Intel GPU. This is the same problem on mobile and tablets where the old behavior was to ask an Adreno 420 to render at 2560x1440 which resulted in 15fps on my phone and 30fps on my Nexus 9 with nothing but a blank globe.
  2. Rendering at higher resolution eats up way more battery, which is equally important on laptops and mobile devices.
  3. Bandwidth issues as mentioned in Honor windows.devicePixelRatio by default #3233.

We need to figure out how to resize without smoothing, which is easy to do with a 2D context, but I'm not sure how to do it with 3D (one option could be render to 3D, copy to 2D and then resize without smoothing.)

@mramato
Copy link
Contributor

mramato commented Nov 27, 2015

Can you try disabling the FXAA pass and see how that looks? I think applying FXAA and then resizing may be part of the problem. One possible solution would be to disable FXAA if resolutionScale !== 1.0 but I think we can do better than that.

@mramato
Copy link
Contributor

mramato commented Nov 27, 2015

Also, I'm fine with backing out #3233 for 1.16 if we'd rather just keep the status quo for now, but the overall issue is something we've let go on for to long that we should address soon.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 27, 2015

It looks worse without fxaa.

With fxaa:

image

Without fxaa:

image

Also, I'm fine with backing out #3233 for 1.16...

Let's do this and revisit as time allows.

@kring
Copy link
Member

kring commented Nov 27, 2015

I think the problem is that #3233 is wrong. It changed this:

var zoomFactor = defaultValue(window.devicePixelRatio, 1.0) * widget._resolutionScale;

To this:

var zoomFactor = (1.0 / defaultValue(window.devicePixelRatio, 1.0)) * widget._resolutionScale;

So previously, with a devicePixelRatio of 2.0 (like on a Retina MacBook), zoomFactor would be 2, so the buffer width would be twice the canvas width (in CSS pixels).

Now, when devicePixelRatio is 2.0, the zoomFactor is 0.5, so the buffer width is half the canvas width in CSS pixels. Making each buffer pixel 4x4 screen pixels instead of the intended 2x2. It shouldn't be 0.5, it should be 1.0. So the correct thing is to ignore the devicePixelRatio in computing zoomFactor, like this:

var zoomFactor = widget._resolutionScale;

@mramato
Copy link
Contributor

mramato commented Nov 28, 2015

Thanks @kring, you're totally right. I don't know how I screwed that up. I'm going to open a new PR with the fix, and hopefully it will look good enough to ship on @pjcozzi's machine.

mramato added a commit that referenced this issue Nov 28, 2015
As @kring pointed out in #3249, my code in #3233 was actually wrong and
caused increased resolution loss when `window.devicePixelRatio` was greater
than 1.  We should just be ignoring `window.devicePixelRatio` completely
because the browser takes care of it by default.
@mramato
Copy link
Contributor

mramato commented Nov 30, 2015

For those that didn't see #3251, even the fixed code is blurry because of browser smooth scaling of WebGL canvases. However, thinking about this some more, could we simply render to a texture at the browser-requested size and then draw that on a full-screen quad without smoothing? That seems like it would fix the problem completely and be easy to implement.

mramato added a commit that referenced this issue Nov 30, 2015
As discussed in #3249.
@mramato mramato mentioned this issue Nov 30, 2015
@mramato
Copy link
Contributor

mramato commented Nov 30, 2015

I spoke with @bagnell offline and he thinks implementing the idea in my last post should be simple, since we are already rendering to a texture. He's going to look into it when he has spare time.

@lilleyse
Copy link
Contributor

It looks like this change affects the screen space error and causes lower detailed imagery to be loaded, which definitely adds to the blurriness. Fog enabled/disabled doesn't make a difference.

Cesium 1.15:
detailed

Current:
blurry

@mramato
Copy link
Contributor

mramato commented Nov 30, 2015

@lilleyse, that's by design. The blurriness is definitely because of the canvas resizing, once that's fixed then we will still have lower-resolution imagery at the same camera view, but it will be crisper.

@mramato
Copy link
Contributor

mramato commented Nov 30, 2015

That being said, this change could also open the door to defaulting maxScreenSpaceError to 1 or 1.5 instead of it's current default of 2.

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

Successfully merging a pull request may close this issue.

4 participants