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

[WIP] Implement native DPI scaling. #86022

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Dec 11, 2023

  • Removes global oversampling in favor of per-viewport oversampling to allow different windows to use different oversampling factor.
  • Implement scalable SVG texture (affected by oversampling factor). Use it for theme element and editor icons.
  • Add support for DPI scaling on macOS and Windows (instead of down scaling for the max DPI screen, content is always rendered at native resolution on all screens).

TODO:

  • LRU for auto-scaled textures.
  • Fix all window/screen coordinate usage.
  • Match scale for embedded windows.
  • Update docs.

doc/classes/TextServer.xml Outdated Show resolved Hide resolved
doc/classes/TextServer.xml Outdated Show resolved Hide resolved
doc/classes/TextServer.xml Outdated Show resolved Hide resolved
doc/classes/TextServer.xml Outdated Show resolved Hide resolved
doc/classes/TextServer.xml Outdated Show resolved Hide resolved
doc/classes/FontFile.xml Outdated Show resolved Hide resolved
doc/classes/SystemFont.xml Outdated Show resolved Hide resolved
@Calinou
Copy link
Member

Calinou commented Dec 11, 2023

Tested locally on https://github.com/godotengine/godot-demo-projects/tree/master/3d/antialiasing, there are some issues when using the canvas_items stretch mode on the root window:

  • Outline size and spacing doesn't seem to be adjusted for oversampling, which means characters get larger but the spacing isn't adjusted to account for it.
  • Popup dialogs are still blurry at high resolutions after resizing the window to a higher size than the default.

Screenshot_20231211_131930

@bruvzg bruvzg force-pushed the viewport_oversampling branch from f17c621 to f4861ae Compare December 12, 2023 07:51
@bruvzg
Copy link
Member Author

bruvzg commented Dec 12, 2023

Outline size and spacing doesn't seem to be adjusted for oversampling, which means characters get larger but the spacing isn't adjusted to account for it.

Should be working now.

#54030 after resizing the window to a higher size than the default.

I'm looking to fix it, but it's not directly related to the oversampling changes (but it should prevent unnecessary cache cleanup and regeneration in cases like this). It's never applied to the child windows, since use_font_oversampling, content_scale_mode and other related properties are only set for the root window (probably should add some sort of "inherited" option for these).

@Calinou
Copy link
Member

Calinou commented Dec 22, 2023

By the way, does this help resolve #76450?

@bruvzg bruvzg force-pushed the viewport_oversampling branch from f4861ae to c6503ef Compare January 3, 2024 08:03
@bruvzg bruvzg changed the title [WIP] Add support for per-viewport font oversampling. [WIP] Implement native DPI scaling. Jan 3, 2024
@bruvzg bruvzg force-pushed the viewport_oversampling branch 11 times, most recently from 3abfb52 to 45cdfbd Compare January 5, 2024 06:07
@bruvzg
Copy link
Member Author

bruvzg commented Jan 10, 2024

FYI: This going to take a bit longer than I have expected. Viewport and various popup related methods are absolute mess, different places are using different (and non-equivalent) transforms and methods to the same thing. And most of it breaks as soon as screen and viewport coordinates have different scale (which is necessary to have consistent multi-monitor handling, due to the way the native API is working).

@bruvzg bruvzg force-pushed the viewport_oversampling branch 2 times, most recently from e9ae758 to d620e85 Compare January 21, 2024 13:38
@MewPurPur
Copy link
Contributor

This would address #41839, right?

@bruvzg
Copy link
Member Author

bruvzg commented Jan 21, 2024

This would address #41839, right?

Yes, this PR should fix it.

@bruvzg bruvzg force-pushed the viewport_oversampling branch 2 times, most recently from e3936c3 to 2931d9e Compare January 31, 2024 18:38
scene/main/viewport.cpp Outdated Show resolved Hide resolved
Comment on lines +1109 to +1116
font_oversampling = content_scale_factor * dpi_scale;
final_size = px_size;
final_size_override = Size2(px_size) / dpi_scale / content_scale_factor;
Copy link
Contributor

Choose a reason for hiding this comment

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

I will need to investigate this further, but I get the feeling, that possibly an additional coordinate system needs to be introduced.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are at least 3-coordinate systems already:

  • Screen coordinates (depends on OS, can be either pixels or scaled, and without one-to-one uniform conversion, sizes can be converted, position can't).
  • Pixel coordinates (for renderer).
  • Viewport scaled coordinates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused on why we even need to consider that there are so many coordinate systems.

Wouldn't most of the "layers" not even know/care about external transforms? I see that this PR adds a screen transform argument to the mouse motion warping for example, which I don't understand.

To try to explain what I mean, consider this: the screen coordinates are simply what the compositor throws at us, while the pixel coordinates are just what we pass to the renderer and the window events.

WaylandThread's internal logic is done entirely in its "native" coordinates, while it converts just at the interface, which are either methods from the DS (screen->pixel) or messages to it (pixel->screen).

Consequentially, the DisplayServer works only with screen coordinates, neither knowing not caring about other transformations.

Can't we just separate concerns like the above case and not worry about the fact that there are other coordinate systems?

Copy link
Member Author

@bruvzg bruvzg Feb 2, 2024

Choose a reason for hiding this comment

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

You can't convert native to pixels at all, it's not uniform if you have multiple displays with different DPIs, and will cause overlaps or holes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bruvzg that's probably why Wayland completely flipped the display model on its head.

We can do that on the Wayland backend as we can't work in screen-coordinates at all, instead doing that only at the window level.

I wonder if we could do a similar thing with DPI handling in general, since I don't see a clean solution to handle heterogeneous monitors otherwise.

Copy link
Member Author

@bruvzg bruvzg Feb 2, 2024

Choose a reason for hiding this comment

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

In general, DPI scaling only need to know window size (in pixels) and scale. Screen coordinates are only necessary for setting window positions, and initial/min/max size of the window (which is still not fully done it this PR, since some parts of the engine are setting size before the window is shown, and it's unknown what scale it will have, so I'll probably have to add methods to set it in both screen and viewport coords).

For the reference:

  • on Windows (and X11) all window/screen coordinates are pixels. You can get current screen DPI and calculate scale from it.
  • on macOS, window/screen coordinates are scaled native coordinate. You can get current window scale (and use it to determine the size of the window in pixels in its current state, and get a signal when scale is changing).

Copy link
Contributor

@Riteo Riteo Feb 2, 2024

Choose a reason for hiding this comment

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

Mh all right. So I suppose that the new coordinate system would be just for the screen stuff? I'm struggling to understand what's the whole issue with different transforms and coordinate systems since most interaction between those shouldn't be direct I think.

on macOS, window/screen coordinates are scaled native coordinate. You can get current window scale (and use it to determine the size of the window in pixels in its current state, and get a signal when scale is changing).

FTR, this sounds pretty close to how it works with Wayland AFAICT. We get some "logical" size, which is kinda-pixel based (it works with scaled pixels so for all intents and purposes they are pretty much a thing by themselves), and then calculate the actual HiDPI buffer from the scale, depending on which of current two scale systems is being used (buffer or fractional).

Comment on lines 76 to 77
Size2i popup_size = get_screen_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely familiar with EDSCALE and the per-window-scaling.
I would appreciate, if you could tell me, if my interpretation of this code section is correct:

The intention of the code is to create a Window with a viewport size of 900x700 pixel. This size is adjusted by EDSCALE. So EDSCALE=2 means that the windows viewport should have a size of 1800x1400.

In order to allow per-window-scaling, it becomes necessary, that the each window can additionally be adjusted by a factor.
When the per-window-scaling has for example a value of 1.5, then the effect is, that a window with a 1800x1400 viewport takes up 2700x2100 pixel on the screen (or its embedder if embedded).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, your calculations are correct.

EDSCALE is a user adjustable scale that is applied on top of window scale (and it's global, not pre-window/display). It's currently used to adjust editor size to DPI in some cases, but with window scaling should be 1 in the majority of cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the quick reply! Then the above code section should be changed to

Suggested change
Size2i popup_size = get_screen_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);
Size2i popup_size = get_final_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);

get_screen_transform doesn't work for embedded windows because it calculates the transform between the windows viewport and the OS-screen (even if the window is deeply nested in other viewports), so it is necessary to use a function, that returns the transform between the windows embedder (could be the OS-screen or a different Viewport) and the windows viewport. get_final_transform() is intended to be this function.

Could it be a long-term plan to make EDSCALE work by using per-window-scaling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed all theses to final_transform.

Copy link
Member Author

Choose a reason for hiding this comment

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

After the last rebase, get_final_transform() stopped working at all (for both native and embedded windows), seems like it's no longer include windows position and taking content scale into amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will look into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

A) Window.get_final_transform() is not supposed to include the windows position in its embedder and never did, as far as I am aware.
B) I need to investigate Window::content_scale_factor in detail when I have more time.

scene/main/window.cpp Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
scene/gui/popup_menu.cpp Outdated Show resolved Hide resolved
scene/main/window.cpp Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
Comment on lines 76 to 77
Size2i popup_size = get_screen_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the quick reply! Then the above code section should be changed to

Suggested change
Size2i popup_size = get_screen_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);
Size2i popup_size = get_final_transform().basis_xform(Vector2i(900 * EDSCALE, 700 * EDSCALE));
popup_centered_clamped(popup_size, 0.8);

get_screen_transform doesn't work for embedded windows because it calculates the transform between the windows viewport and the OS-screen (even if the window is deeply nested in other viewports), so it is necessary to use a function, that returns the transform between the windows embedder (could be the OS-screen or a different Viewport) and the windows viewport. get_final_transform() is intended to be this function.

Could it be a long-term plan to make EDSCALE work by using per-window-scaling?

@bruvzg bruvzg force-pushed the viewport_oversampling branch from 2931d9e to ccf63c8 Compare February 6, 2024 10:24
@bruvzg bruvzg force-pushed the viewport_oversampling branch from ccf63c8 to 22f4ca0 Compare February 14, 2024 22:03
@lostminds
Copy link

lostminds commented Feb 15, 2024

The auto-scaling SVGTextures are very interesting to me, I've made something similar for displaying scaled SVG content using existing SVG image generation functionalities. But it's not very efficient and quite complicated tracking scale changes and it requires generating a new image and texture on each scale change, so I'd really love to have something more efficient and better integrated in the engine.

However, my version does have one key functionality this new SVGTexture seems to miss: async updating. If I've understood your code correctly SVGTexture::_ensure_scale() is called on draw and this is where a new SVG image is rendered and a texture is generated for this scale if there isn't one. However, ImageLoaderSVG::create_image_from_string can be quite slow for more complex images. So what I'd really love to see for my use case would be an option to do this rendering in the background. Basically ensure that there is at least a first rendered size of the texture on load. In _ensure_scale, instead pick the closest suitable scaled version from existing ones, and when appropriate instead start generating a new image/texture on a background thread and add that to the list of scaled textures once it's rendered (which will cause it to start being used then in rendering). In other words keep using a different sized texture while the desired scaled texture is being rendered in the background instead of blocking.

@lostminds
Copy link

When rendering the SVGTexture ImageLoaderSVG::create_image_from_string and the underlying functions first loads/parses the SVG content into a ThorVG Picture representation of some sort. This representation is then rendered into a pixel buffer of the desired size for the image texture. Since the SVGTexture will be redrawing the same SVG content it seems like it should be possible to reuse the parsed representation instead of reloading it from the SVG string each resize.

Maybe the SVGTexture could be complemented with an SVGImage class used for this purpose? Basically a subclass of Image that retains the parsed svg representation to allow re-rendering the same svg content at a different resolution without needing to parse it from string. For simple SVGs it might not make much difference, but complex svgs can contain a lot of stuff like base64-encoded bitmap images, embedded fonts etc where the parsing/loading is a bottleneck it would be nice to avoid on each re-render.

Implement scalable SVG texture. Use it for theme element and editor icons.
Add support DPI scaling on macOS and Windows.
Add support for per-viewport font oversampling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants