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

Ability to render the viewer content at quarter resolution on very-high-DPI screens #2321

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Feb 12, 2024

Motivation and Context

Changes needed in the native viewer application and the RenderTarget to allow for rendering at a resolution that's smaller than the actual framebuffer. For simplicity halving the framebuffer width and height, thus quarter of the size, however the limit at which this switch is done is configurable, so it's also achievable on non-HiDPI displays if you pass --quarter-resolution-scaling-limit 1 to the app.

It also:

  • Removes use of 4x MSAA which achieved nothing as the content was rendered to a non-multisampled framebuffer in the first place.
  • Cleans up various interesting ways of doing vector math.
  • Removes an assertion in the RenderTarget that required the source and destination size to match, and switches the filter to linear to not look that blocky.
  • Removes an errorneous multiplication by dpiScaling() when getting a framebuffer-relative mouse position. The DPI scaling value is already baked into the framebuffer size at that point, thus multiplying again gives an incorrect value.

See also https://cvmlp.slack.com/archives/CFN5TAUSD/p1697296903351319 for original context.

How Has This Been Tested

On my Linux box with a 4K display, 1.6875x DPI scaling and --quarter-resolution-scaling-limit 1.6. The way the limit calculation is done it should work on a Mac as well.

Types of changes

  • New feature (non-breaking change which adds functionality)

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 12, 2024
Copy link
Contributor

@jturner65 jturner65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks mosra!

Copy link
Contributor

@eundersander eundersander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor request, otherwise LGTM. This is mainly reference code for our other GUI apps floating around (like the HITL apps) where we'll reimplement this logic. Thanks for doing this!

@@ -863,11 +864,25 @@ Viewer::Viewer(const Arguments& arguments)
.addOption("agent-transform-filepath")
.setHelp("agent-transform-filepath",
"Specify path to load camera transform from.")
.addOption("quarter-resolution-scaling-limit", "2.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a more intuitive interface to expose to the command line would be a simple bool, something like "--reduce-resolution-for-high-dpi-displays". Which should probably set the scaling limit to something like 1.6 or 1.9, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternately, at least a better description for this arg so people know how to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the thing, I didn't expect people to use this option :) The default behavior makes sense to me. The option is there just as a fallback, to tune when the size reduction kicks in, in rare cases such as when the desktop scaling is configured weird, or when the GPU is not powerful enough to render at full resolution, or when making screenshots and so you do want a full resolution even on a HiDPI screen.

As I understood the request, the main objective was to reduce the resolution by default on very-high-DPI displays, which is for me with 200% display scale and higher. On lower density displays with 1.5x or 1.6x scaling it looks visibly uglier (as it did on my 4K 23" screen), so there it isn't enabled, while for >=2x scaled displays you'll see the difference only if you really focus on it (as it was the case on my 4K 16" display).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I understand now. quarter-resolution-scaling-limit==2.0 means "reduce resolution for very-high-DPI displays" (by which we mean Retina and other 200%+ display scales). quarter-resolution-scaling-limit==9999 would mean "don't reduce resolution, regardless of DPI". quarter-resolution-scaling-limit==1.0 would mean "always reduce resolution, regardless of DPI".

Anyway I don't think we particularly need this feature for the C++ viewer, which isn't used much any more. This is a helpful reference. At some point, @0mdc or I can port this to the HITL framework's replay-renderer, where Retina has been bothering us.

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The actual content is drawn to a non-MSAA framebuffer and then blitted,
so this achieves nothing at all, and prevents framebuffer content
scaling during the blit.
Thus no longer assert that the source and destination sizes match, and
blit with a linear filter instead.

The 2x limit is configurable with --quarter-resolution-scaling-limit.
@jturner65 jturner65 merged commit ff7f0be into main Mar 1, 2024
9 of 10 checks passed
@jturner65 jturner65 deleted the framebuffer-downscale branch March 1, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants