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

[3.x] Add a 2D scale factor property #52137

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

Ansraer
Copy link
Contributor

@Ansraer Ansraer commented Aug 26, 2021

This can be used to make 2D elements larger or smaller, independently of the current stretch mode.
Only the disabled and 2d stretch modes support this new property.

This is necessary to properly render 2D UI elements on high res monitors. I have used this myself for a couple of UI-heavy projects, and so far it has worked very well on my 4k monitor. The only real problem I experienced was image scaling, but to fix that we will need godotengine/godot-proposals#2924.

3.x only for now, since the last time we discussed this @Calinou said that he would look into porting this to master himself because apparently some of the underlying logic changed. See #52170.

DemoProject

Rebase of #21446


Btw, I am sorry that it took me so long to PR a simple rebase, was on vacation for the last few weeks, and attempts to get work done on my laptop failed spectacularly. 😕

@Ansraer
Copy link
Contributor Author

Ansraer commented Sep 29, 2021

Adjusted the commit message according to Akien's request on rocketchat.

@Ansraer
Copy link
Contributor Author

Ansraer commented Sep 29, 2021

After discussing this PR on rocketchat with reduz and groud we agreed that having two properties (scale and shrink) that do the same thing is not ideal.
Instead, I reworked the already existing shrink property so that it now can also be used to scale things up. This way we don't break compatibility for existing users.

Here is a new demo project:
2d-scaling-demo.zip

EDIT: Accidentally pressed ctrl+enter while writing this comment. Please ignore the close/reopen below.

@Calinou
Copy link
Member

Calinou commented Sep 29, 2021

Instead, I reworked the already existing shrink property so that it now can also be used to scale things up. This way we don't break compatibility for existing users.

Note that this isn't the same when using the viewport stretch mode, but this can be worked around manually. I don't think UI scaling independently of the viewport size is very common in a pixel art game anyway.

scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
Comment on lines 1112 to 1113
root->set_size_override_stretch(true);
root->set_size_override(true, (last_screen_size / (stretch_scale)).floor());
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a bit weird that STRETCH_MODE_DISABLED sets the size_override_stretch to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, without enabling stretch it would be impossible to scale things up. However, the only stretching that happens is the user-defined scaling, so I would argue that there is no automatic scaling.
Perhaps STRETCH_MODE_MANUAL would be a better name than STRETCH_MODE_DISABLED but that would probably be a rather confusing and major change.

scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
@@ -217,7 +217,7 @@
<argument index="2" name="minsize" type="Vector2" />
<argument index="3" name="shrink" type="float" default="1" />
<description>
Configures screen stretching to the given [enum StretchMode], [enum StretchAspect], minimum size and [code]shrink[/code] ratio.
Configures screen stretching to the given [enum StretchMode], [enum StretchAspect], minimum size and [code]shrink[/code] (which can also be used to scale the content up).
Copy link
Member

Choose a reason for hiding this comment

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

Would also be good to add a comment in the project setting docs that this can also be used for upscaling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, you are talking about godotengine/godot-docs/blob/stable//classes/class_projectsettings.rst, right? I am somewhat confused since so far I always assumed that this part of the docs was auto-generated, yet when I checked it (and the underlying ProjectSettings.xml file) just now there was no mention of any of the stretch settings. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about doc/classes/ProjectSettings.xml where all project settings are normally documented.

But here indeed the stretch settings are missing: https://github.com/godotengine/godot/blob/3.x/doc/classes/ProjectSettings.xml#L488-L490

That must mean that they are only defined under specific conditions which are not fulfilled when generating documentation with doctool. The GLOBAL_DEF calls should be moved somewhere else so that they appear in the docs. But that's not blocking for this PR then as it's only tangentially related.

scene/main/scene_tree.h Outdated Show resolved Hide resolved
@reduz
Copy link
Member

reduz commented Oct 5, 2021

Technically this seems good, the only concern is that this breaks compat. @akien-mga If you are ok with this feel free to merge.

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.

I think it's fine compat wise as we only renamed the argument for set_screen_stretch, but default behavior is the same.

@akien-mga akien-mga merged commit 77f52bd into godotengine:3.x Oct 5, 2021
@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.

4 participants