-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Conversation
CC @Calinou (I know that you don't have time to work on this, but I think that you'd like just seeing this :) ) |
"scaling mode" may be confused with the 3D scaling project settings.
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 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. |
Is that an issue with 2D?
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 think I'll move in this direction then if no one else is against. I'll update the PR later. |
Yes, as many people will attempt to tweak the wrong setting 🙂
Not yet, as this has been attempted and it caused crashes in the past. You have to call 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. |
@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 🙂 |
@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. |
363471c
to
3e4f2f0
Compare
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. |
Tested this locally (rebased against The only issue is that using the 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 Nonetheless, I think this is mergeable already after applying suggestions. We can improve the Testing project: multiple_resolutions.zip Screenshots (648×648 base window size, 3840×2160 window size)
|
@Calinou I somehow missed your review, sorry! I'll address this stuff as soon as I have time, thanks for testing! |
Sorry for the DP, but @Sauermann thank you too for testing! |
@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. |
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. |
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
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.)
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. |
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"? |
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. |
I meant that "stretch mode" is already used elsewhere: godot/core/config/project_settings.cpp Line 1318 in 2210111
|
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"? |
@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. |
There was a problem hiding this 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.
There was a problem hiding this 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".
b6e6f3e
to
87cfc41
Compare
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>
Thanks! |
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 if
s with justif
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.
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 (Update: went for.../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.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.