-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Reconsider image scaling and fxaa #7875
Comments
FXAA
👍 Do you have a screenshot for just FXAA disabled? Also in the future, we may use other AA approaches. Image ScalingFor disabling image scaling, I am not concerned about the GPU performance, but I am concerned about the increase the in the number of tile requests.
Yes or change the default SSE
Doesn't seem super predictable for developers using Cesium. Hybrid
Sounds promising but maybe there is low hanging fruit with the above two to start? |
Updated the post with a just-FXAA disabled screenshot. The purple label looks better but the white label still has issues.
As long as we aren't just increasing the default SSE to 32 or something, but instead multiplying by the device pixel ratio.
@bagnell's conclusion in #4235 was that it would get pretty complicated, so I am definitely in favor of a shorter term solution. |
I uploaded one more image above with image scaling disabled, FXAA disabled, and SDF. This one looks the best to me. The white label outline is less strong and overall seems clearer. |
Looking at the new images:
What do you think? |
That sounds good to me. |
While I'm mostly okay with everything here (if we don't care about hurting frame rate on mobile and under-powered GPU/Monitor combos in order to make it look better on the majority of machines) but what I don't understand is:
Do we know what the average screen resolution for users is? I bet it's well below 4k and most users (outside mobile) have a devicePixelRatio of 1.0. If that is indeed the case, disabling image scaling will be a no-op for most users and raising the SSE will simply make less tiles render with no change in resolution. I think we need to keep the same SSE or at least tie the default SEE to devicePixelRatio so that we don't reduce it needlessly. |
Yes we're in agreement, my comment above is about that same point
|
Awesome, sorry I missed that. I'm actually excited about these changes because I think it will help Cesium shine. |
To recap the above discussion, the change we all agreed with here is:
|
Fixed in #8083 |
@pjcozzi and I were talking about SDF fonts and how Cesium's default rendering may be hurting image quality more than SDF can help. If our goal is crisp rendering we may want to disable image scaling and FXAA. I created some screenshots to show how image quality can improve by disabling these features. They were captured on a 4K monitor with 1.3x display scaling. Make sure to view them in full resolution.
default
viewer.resolutionScale = window.devicePixelRatio;
viewer.scene.postProcessStages.fxaa.enabled = false;
viewer.resolutionScale = window.devicePixelRatio;
viewer.scene.postProcessStages.fxaa.enabled = false;
viewer.resolutionScale = window.devicePixelRatio;
viewer.scene.postProcessStages.fxaa.enabled = false;
SDF
If we disable image scaling there will be a performance hit in both the number of shaded pixels and number of tiles requested. I haven't ran performance tests yet. In the 3D Tiles Inspector sandcastle I saw a difference of 316 vs. 453 tiles visited. One compromise might be to disable image scaling for rendering but still account for it when computing screen space error. Another compromise might be to use image scaling on mobile devices but not laptops/desktops if that's knowable.
FXAA seems like something we should just disable by default. It's more likely that someone will get turned off by blurry text than pixelated edges.
Then there's a hybrid solution which is to render the scene as normal with image scaling and fxaa but render billboards to a higher resolution target without anti-aliasing. #4235 goes into more details about that.
Mainly I'm just looking for discussion and whether these are changes we'd be comfortable making.
Also see #7682
The text was updated successfully, but these errors were encountered: