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

"pixelperfect" window aspect. #46061

Closed
wants to merge 1 commit into from

Conversation

bfloch
Copy link
Contributor

@bfloch bfloch commented Feb 15, 2021

This mode makes sure that only integers are used as multiplier or divisor
for the viewport size, hence achieving "pixel perfect" scaling.
This works best with the viewport stretch mode.

As opposed to the other aspects this mode will allow for black borders
on all sides to satisfy the requirement.

It achieves a setup similar to https://alvarber.gitlab.io/pixel-perfection-in-godot.html
but just with a single setting as shown:
pixelperfect aspect

Since the godot build is broken for me in master/4.0 as reported by others in #43714 and #42348 I was not able to test this on master.

But I do have a sister patch for 3.2 in case you prefer this small, but useful feature for 3.x here:
StateOff@a63c016

Video showcasing the effect in 3.2:
https://i.imgur.com/kBb3Y49.mp4

kBb3Y49.mp4

For convenience here is a stable 3.2.3 + patch Windows build for the curious without access to a compiler:
https://github.com/StateOff/godot/releases/tag/3.2.3-pixelperfect-0.1

Since I have limited access to platforms beyond Linux let me know if the patch needs more love.

Bugsquad edit: This closes godotengine/godot-proposals#1666.

This mode makes sure that only integers are used as multiplier or divisor
for the viewport size. This works best with viewport stretch mode.
As opposed to the other aspects this mode will allow for black borders
on all sides to satisfy the requirement.
@bfloch bfloch force-pushed the pixelperfect_master branch from 44eb8d3 to 50ee2f0 Compare February 15, 2021 18:39
@bfloch
Copy link
Contributor Author

bfloch commented Feb 15, 2021

^ Force pushed std:: -> Math:: change.

@Calinou
Copy link
Member

Calinou commented Feb 15, 2021

This is a good feature, but I don't think this should be a stretch aspect setting. For instance, you may want to support multiple aspect ratios while having pixel-perfect scaling. However, with the approach in this PR, you can't use both the expand and pixelperfect stretch aspect modes at the same time.

Having a new project setting dedicated to integer scaling would be a more universal approach to this problem. It would complement #21446 nicely. It'd also work better when using the 2d stretch mode, and even when using the disabled stretch mode when you want to force a specific UI scale.

I think this could be merged as-is in the 3.2 branch as a stop-gap solution, but we should spend more time thinking about this for the master branch.

@Calinou Calinou added this to the 3.2 milestone Feb 15, 2021
@bfloch
Copy link
Contributor Author

bfloch commented Feb 15, 2021

Thanks for the feedback. It is a valid point - I did not anticipate the need for other aspects, frankly I have not seen this myself, but I understand that we should keep the engine as generic as possible and let the user define the use-cases.

Also I haven't visited the existing efforts and requests, so thanks for connecting the dots here.

I am ok with putting so more work into this as suggested, my only concern currently is that I can not run the master until some of the mentioned issues are resolved since I do not have a Mac or even Windows partition in my household at this point.

I am trying to suggest some patches which might ease the situation but there seems to be more work necessary to get 4.x somewhat stable on my platform at this point. So maybe it is best to postpone the work at bit.

@dalexeev
Copy link
Member

This needs to be implemented either as a mode or as a separate property, but not as an aspect. See Yukitty/godot-addon-integer_resolution_handler.

Comment on lines +578 to +586
if (video_mode >= desired_res) {
Size2 scale_size = video_mode / desired_res;
int scale_factor = Math::floor(std::min(scale_size.x, scale_size.y));
screen_size = desired_res * scale_factor;
} else {
Size2 scale_size = desired_res / video_mode;
int scale_factor = Math::ceil(std::max(scale_size.x, scale_size.y));
screen_size = desired_res / scale_factor;
}
Copy link
Member

Choose a reason for hiding this comment

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

We have our own MIN and MAX macros which should be used instead of std.

@akien-mga akien-mga requested a review from reduz February 24, 2021 11:51
@akien-mga akien-mga modified the milestones: 3.2, 4.0 Feb 24, 2021
@akien-mga akien-mga requested a review from groud February 24, 2021 11:58
@groud
Copy link
Member

groud commented Feb 24, 2021

Aside from @akien-mga's comment, this looks good to me.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023
@Riteo
Copy link
Contributor

Riteo commented Apr 5, 2023

This one looks to have consensus and to be pretty clean, only needing to apply some trivial changes and a (hopefully simple) rebase. Is the author still active? Otherwise I'd be glad to salvage this.

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

See #46061 (comment). I think this PR acts more of a stopgap than a definitive fix for godotengine/godot-proposals#1666, so I'd prefer we spend more time working on a more durable solution (so we don't have to deprecate something soon after introducing it).

This could be worth salvaging as a 3.x PR for 3.6 though (see the link in OP), since we don't have as much resources to perform heavy-duty changes there.

@Riteo
Copy link
Contributor

Riteo commented Apr 5, 2023

@Calinou uh, somehow I didn't see that comment, I even thumbed it up who knows how much time ago! So, I guess that this at this point could be superseded by a proper integer scaling PR, I might take the lead if nobody's against that, sounds fun :P

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

I have a PR that attempted to add integer scaling in a more flexible way, but didn't manage to finish it (and have no time to do so): #63206

@Riteo
Copy link
Contributor

Riteo commented Apr 5, 2023

@Calinou oh, I didn't know that, sorry. If you don't have time might I take over that one if I find some time myself? I'd be interested by such a feature and implementing it would be a pleasure if nobody wants/can.

Edit: the PR asks for help lmao, I should read before asking, I'll risk and take that as a yes :P

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

If you don't have time might I take over that one if I find some time myself?

Sure, go ahead 🙂

This will be very appreciated by the community, thanks for looking into it!

@Calinou
Copy link
Member

Calinou commented Jul 28, 2023

@Calinou Calinou closed this Jul 28, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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
7 participants