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

Add content scale stretch modes, implement integer scaling #75784

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Apr 7, 2023

Supersedes (and based on) #63206
Fixes godotengine/godot-proposals#1666

Integer scaling is achieved (after aspect expansion) by "lying" to the stretching code about the window's size, telling it that it's always an integer multiple of the viewport so that it only gets stretched to an integer factor.

This approach works with all stretch and aspect modes and doesn't require handling for each, only requiring to "loosen up" some self-excluding conditions (in other words, replacing some else ifs with just ifs) regarding viewport offset and margin calculation (black bars).

Includes a tiny usability change that adds a range hint for the content scale factor between 0.5 to 8.0.

Includes stubs for an unimplemented "hybrid" mode (see below), which would allow to fill the whole screen while keeping square pixels with minimal blurring. It might have to be implemented later until core supports more fine-grained window filtering controls.

For hybrid mode see: https://gamingprojects.wordpress.com/2017/12/03/reducing-pixel-blur-and-distortion/ Update: removed.

Notes:

  • I don't really like the current name of this property nor its path (.../window/stretch/stretch), but I can't really think of a good alternative either if not something like "scaling mode", which I'm not sure of either. Update: went for scale_mode.

  • Currently sub-one scales are clamped to one. This means that the window will clip with the aspect ratio calculation still going on, thus making games (especially ones with expanding aspect ratios) pretty unusable if for some bizarre reason the window must stay very small. I'm not sure if I should let the window stretch in these cases. Update: resolved.

  • I feel like this code might be commented/cleaned a bit but I haven't done any since I have no idea what to do with hybrid scaling. I feel like I could seriously remove its stub since it, while being a great feature, would be too much of an hassle right now. Update: resolved.

  • I have tested it both atop of my Wayland branch on my Wayland/AMD laptop and on my Xorg/NVIDIA desktop. Both seemed to work fine (although the desktop was super slow at resizing but I recall that it was normal, dunno, I haven't used it for serious godot development in ages). That said, more serious and thorough testing is always appreciated.

@Riteo
Copy link
Contributor Author

Riteo commented Apr 7, 2023

CC @Calinou (I know that you don't have time to work on this, but I think that you'd like just seeing this :) )

@Calinou
Copy link
Member

Calinou commented Apr 7, 2023

  • I don't really like the current name of this property nor its path (.../window/stretch/stretch), but I can't really think of a good alternative either if not something like "scaling mode", which I'm not sure of either.

"scaling mode" may be confused with the 3D scaling project settings.

  • Currently sub-one scales are clamped to one. This means that the window will clip with the aspect ratio calculation still going on, thus making games (especially ones with expanding aspect ratios) pretty unusable if for some bizarre reason the window must stay very small. I'm not sure if I should let the window stretch in these cases.

I think it's fine this way, as long as it's documented in the class reference. We can recommend setting the minimum window size to the base window size if you want to avoid display issues.

  • I feel like this code might be commented/cleaned a bit but I haven't done any since I have no idea what to do with hybrid scaling. I feel like I could seriously remove its stub since it, while being a great feature, would be too much of an hassle right now.

I agree with removing hybrid mode from this PR, but keep the property as an enum so that more options can be added without breaking compatibility.

@Riteo
Copy link
Contributor Author

Riteo commented Apr 7, 2023

@Calinou

"scaling mode" may be confused with the 3D scaling project settings.

Is that an issue with 2D?

I think it's fine this way, as long as it's documented in the class reference. We can recommend setting the minimum window size to the base window size if you want to avoid display issues.

That would probably be the best option. Is there a project setting for setting the minimum window's size? I recall not being able to find it before.

I agree with removing hybrid mode from this PR, but keep the property as an enum so that more options can be added without breaking compatibility.

I think I'll move in this direction then if no one else is against. I'll update the PR later.

@Calinou
Copy link
Member

Calinou commented Apr 7, 2023

Is that an issue with 2D?

Yes, as many people will attempt to tweak the wrong setting 🙂

That would probably be the best option. Is there a project setting for setting the minimum window's size? I recall not being able to find it before.

Not yet, as this has been attempted and it caused crashes in the past. You have to call DisplayServer.window_set_min_size() in an autoload's _ready() function instead.

It's possible that this setting can be reintroduced just fine with 4.0's DisplayServer still.

@Riteo
Copy link
Contributor Author

Riteo commented Apr 8, 2023

@Calinou

Not yet, as this has been attempted and it caused crashes in the past. You have to call DisplayServer.window_set_min_size() in an autoload's _ready() function instead.

It's possible that this setting can be reintroduced just fine with 4.0's DisplayServer still.

I'm pretty sure that Godot already sets a minimum window size, so I suppose that whatever issue caused these crashes isn't there anymore. Anyways we can see in another time.

@Riteo
Copy link
Contributor Author

Riteo commented Apr 8, 2023

@Calinou I see that in the original PR you added a range hint to the content scale factor property. Why's that? Should I remove/keep it?

@Calinou
Copy link
Member

Calinou commented Apr 8, 2023

@Calinou I see that in the original PR you added a range hint to the content scale factor property. Why's that? Should I remove/keep it?

It was an additional usability improvement, as the setting previously lacked any range hint. Only values greater than 0.5 really make sense to use, and values above 8.0 are probably too large for any project. I suggest keeping it in this PR, but it could be moved to another PR in theory 🙂

@Riteo
Copy link
Contributor Author

Riteo commented Apr 8, 2023

@Calinou mh, I'd be tempted to make this a separate tiny PR, but I guess that since this is a content scale related PR it might make sense to make this tiny change.

I'll update the commit and the PR to document this change.

@Riteo Riteo force-pushed the int-scale branch 2 times, most recently from 363471c to 3e4f2f0 Compare April 8, 2023 16:15
@Riteo
Copy link
Contributor Author

Riteo commented Apr 8, 2023

Well, I think we're pretty close to mark this as ready for review. I've tried to add some documentation but I'm not sure whether it's good enough. Other than that we should be done.

I've also taken a look around the scaling code to see if I could clean up something, but everything looks both pretty intertwined and important, so I'd avoid changing things willy-nilly without being extremely sure to not have broken something. This also has the advantage of a smaller and cleaner diff, which never hurts.

Edit: wait, if the only thing left is making sure that documentation is good that means that this is ready for review! Silly me. Marking it now as such.

@Riteo Riteo marked this pull request as ready for review April 8, 2023 17:07
@Riteo Riteo requested review from a team as code owners April 8, 2023 17:07
@Riteo Riteo changed the title WIP: Add content scale stretch modes, implement integer scaling Add content scale stretch modes, implement integer scaling Apr 8, 2023
@YuriSizov YuriSizov added this to the 4.x milestone Apr 10, 2023
@Calinou
Copy link
Member

Calinou commented Apr 26, 2023

Tested this locally (rebased against master as of today). It works as I anticipated, with one exception:

The only issue is that using the 2d stretch mode with integer scaling does not fully stretch the viewport to match the window size. What I expect is that 2D scaling will always be an integer factor, but the viewport size should match the window size (so that there are no black borders, and 3D rendering is visible across the entire window). This requires project developers to configure UI anchors correctly, but this is already an expectation when using the 2d stretch mode in general (or even disabled).

This is generally what 3D games do when they have a pixel art aesthetic but want to keep rendering at native resoluton, such as Minecraft. Fortunately, you can emulate this by using the disabled stretch mode, integer scale mode and setting the content scale factor based on float(window_height) / base_window_height. Godot will then floor() the resulting scale factor, ensuring it's always integer.1

Nonetheless, I think this is mergeable already after applying suggestions. We can improve the 2d stretch mode later as it can be manually emulated as I explained. Great work 🙂

Testing project: multiple_resolutions.zip

Screenshots (648×648 base window size, 3840×2160 window size)

2d, fractional

Screenshot_20230426_191905

2d, integer

Screenshot_20230426_191911

viewport, fractional

Screenshot_20230426_191916

viewport, integer

Screenshot_20230426_191921

disabled, fractional, scale factor set to 200%

The result looks identical regardless of whether you set the scale mode to fractional or integer in this particular case, since 200% is already an integer scale factor.

Screenshot_20230426_191952

Footnotes

  1. This can be done even without this PR, but you'll have to floor() the resulting scale factor yourself.

doc/classes/Window.xml Outdated Show resolved Hide resolved
doc/classes/Window.xml Outdated Show resolved Hide resolved
doc/classes/Window.xml Outdated Show resolved Hide resolved
@Riteo
Copy link
Contributor Author

Riteo commented May 11, 2023

@Calinou I somehow missed your review, sorry!

I'll address this stuff as soon as I have time, thanks for testing!

@Riteo
Copy link
Contributor Author

Riteo commented May 11, 2023

Sorry for the DP, but @Sauermann thank you too for testing!

@Riteo
Copy link
Contributor Author

Riteo commented May 11, 2023

@Calinou I accepted your suggestions and squashed everything, thanks!

Regarding the 2d scaling remarks, I experimented a bit with what you've done and I'm not entirely sure what you're looking after; that's a bit of a bummer as, since you're saying that this stuff can be emulated just by playing with the settings, the fix shouldn't be hard at all and entirely within the bounds of the window-size-lying approach I'm using.

Anyways, since you're describing this particular thing as non-essential, I agree that we should talk about it thoroughly in another PR so as to avoid stalling this PR further.

@georgwacker
Copy link
Contributor

The only issue is that using the 2d stretch mode with integer scaling does not fully stretch the viewport to match the window size. What I expect is that 2D scaling will always be an integer factor, but the viewport size should match the window size (so that there are no black borders, and 3D rendering is visible across the entire window).

I disagree, that the 2d stretch mode with integer scaling should fully stretch the viewport to match the window.

For a crisp pixel-perfect scaling mode it should just scale the base resolution of the game with the highest possible integer factor and keep it as is - so black borders are expected, if the window size is not a perfect multiple.

I tested this PR with my 2D pixel-art game in 320x180 base resolution (set via root control node) and the behaviour works as expected.

As a compromise, I have an option in my game for a uniform non-pixel-perfect scaling, using viewport stretch and aspect keep to have the game match the window size at least in one dimension. In that mode, pixels are uniformly stretched (for example by 3.3x), but there might be borders in the other dimension (as expected with keep aspect ratio). This already works well.

There could be an additional but slightly different stretch mode, where it uses the highest possible integer factor (let's say 3x), but then stretches the rendered image to the window size, making it slightly blurry, but retaining the initial 3x scale. If I recall correctly, some pixel-art games offered this option, so it might be interesting to test out. But I think this would require a different approach.

As it stands, the PR works great for 2D integer scaling.

I also noticed a slow down / FPS drop when resizing the window with Vulkan, but that also happens on 4.0.3 release with viewport stretch / keep aspect ratio.

@Calinou
Copy link
Member

Calinou commented May 25, 2023

I disagree, that the 2d stretch mode with integer scaling should fully stretch the viewport to match the window.

For a crisp pixel-perfect scaling mode it should just scale the base resolution of the game with the highest possible integer factor and keep it as is - so black borders are expected, if the window size is not a perfect multiple.

Unfortunately, this is a bad approach for 3D games as you don't want black borders to be present on all sides. It unnecessarily limits the visible screen real estate. We don't have a choice when using the viewport stretch mode, but we do when using the canvas_items stretch mode. If you want pixel-perfect behavior for 2D games (with everything aligning to a pixel grid), you'll want to use the viewport stretch mode anyway.

There could be an additional but slightly different stretch mode, where it uses the highest possible integer factor (let's say 3x), but then stretches the rendered image to the window size, making it slightly blurry, but retaining the initial 3x scale. If I recall correctly, some pixel-art games offered this option, so it might be interesting to test out. But I think this would require a different approach.

This is pretty much the hybrid approach I originally mentioned in my PR. (It can also be done by rendering at an integer scale, using bilinear filtering and a sharp-bilinear shader on the final native-resolution viewport.)

I also noticed a slow down / FPS drop when resizing the window with Vulkan, but that also happens on 4.0.3 release with viewport stretch / keep aspect ratio.

Recreating buffers is quite slow, especially in Vulkan. Even if the viewport size remains constant, the window buffer (which must always match the window size) has to be recreated every time you resize the window.

@georgwacker
Copy link
Contributor

georgwacker commented May 26, 2023

Unfortunately, this is a bad approach for 3D games as you don't want black borders to be present on all sides. It unnecessarily limits the visible screen real estate. We don't have a choice when using the viewport stretch mode, but we do when using the canvas_items stretch mode. If you want pixel-perfect behavior for 2D games (with everything aligning to a pixel grid), you'll want to use the viewport stretch mode anyway.

After reading your previous post again, I think I got initially confused by the terminology that the canvas_items stretch mode was called 2D.

I think it would make the most sense, to have the additional hybrid option with integer scaling + filter stretch to work similar for both canvas_items and viewport. So that use-cases for integer scaling and 3D games are covered. That means rendering with integer scale and then stretching the resulting image.

And not have canvas_items + integer work differently than viewport + integer. That means the integer stretch mode from this PR wouldn't make much sense for 3D games as is, but at least it stays consistent on what you would expect an integer scaler would do (including resulting in black bars). I would imagine most use-cases for the pure integer scale vs hybrid are 2D pixel-art games anyways.

I could look into implementing the hybrid mode.

Perhaps "Scaling Mode" would be a better name than "Stretch"?

@Riteo
Copy link
Contributor Author

Riteo commented May 26, 2023

@georgwacker

Perhaps "Scaling Mode" would be a better name than "Stretch"?

IIRC it's already used.

@georgwacker
Copy link
Contributor

Perhaps "Scaling Mode" would be a better name than "Stretch"?

IIRC it's already used.

I've built your branch int-scale two days ago and it's still showing up as "Stretch" under the category Stretch.

@Riteo
Copy link
Contributor Author

Riteo commented May 26, 2023

@georgwacker

I've built your branch int-scale two days ago and it's still showing up as "Stretch" under the category Stretch

I meant that "stretch mode" is already used elsewhere:

GLOBAL_DEF_BASIC(PropertyInfo(Variant::STRING, "display/window/stretch/mode", PROPERTY_HINT_ENUM, "disabled,canvas_items,viewport"), "disabled");

@georgwacker
Copy link
Contributor

I meant that "stretch mode" is already used elsewhere:

Ah, gotcha. Well, it could still be possible to have this option as "display/window/stretch/scaling_mode" in ProjectSettings, but if we want to avoid another mode named setting, maybe "Scaling method"?

@Riteo
Copy link
Contributor Author

Riteo commented May 28, 2023

maybe "Scaling method"?

@georgwacker that's a bit confusing too.

Personally I think that the whole category should be renamed from scratch but we can't as that would break compatibility so I'd just not worry too much... It's all a mess honestly.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I've retested this PR (rebased on top of master f8dbed4), it works as expected.

I think we can go with this solution. Even if it's not perfect (especially for 3D games), it's much better than the status quo. For 3D games, I can document the currently possible solution with the disabled stretch mode + a method called on resize that changes the content scale factor. It's made a bit easier by this PR as you no longer have to snap the value to integers yourself. For 3D rendering itself, godotengine/godot-proposals#4697 is a more flexible solution.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Asked reduz for opinions, he said "makes sense I guess".

@Riteo Riteo force-pushed the int-scale branch 2 times, most recently from b6e6f3e to 87cfc41 Compare August 10, 2023 17:42
Integer scaling is achieved (after aspect expansion) by "lying" to the
stretching code about the window's size, telling it that it's always an
integer multiple of the viewport so that it only gets stretched to an
integer factor.

This approach works with all stretch and aspect modes and doesn't
require handling for each, only requiring to "loosen up" some
self-excluding conditions (in other words, replacing some `else if`s
with just `if`s) regarding viewport offset and margin calculation (black
bars).

Includes a tiny usability change that adds a range hint for the content
scale factor between 0.5 to 8.0.

Co-Authored-By: Hugo Locurcio <hugo.locurcio@hugo.pro>
@akien-mga akien-mga merged commit 33198d0 into godotengine:master Aug 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Add a project setting to force an integer scale for window stretch
6 participants