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

always set "logical size" for render #1048

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

Conversation

rfomin
Copy link
Collaborator

@rfomin rfomin commented Apr 12, 2023

This fixes screen stretching if aspect_ratio_correct == 0.

Before:
DOOM0005
After:
DOOM0004

The size of the window in windowed mode is not corrected, should we change it too?

This fixes screen stretching if `aspect_ratio_correct == 0`.
@SoDOOManiac
Copy link
Collaborator

SoDOOManiac commented Apr 13, 2023

I'm not sure if this should be done at all, for square pixels we have aspect_ratio_correct == 2.

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 13, 2023

I'm not sure if this should be done at all, for square pixels we have aspect_ratio_correct == 2.

Wow, this is a super secret option, I didn't know about it. But why do we need aspect_ratio_correct == 0? For users, it looks broken.

@SoDOOManiac
Copy link
Collaborator

aspect_ratio_correct == 0 makes sense for scaling the framebuffer to old 1280x1024 displays, their aspect ratio is 1.25 which is less than the 'classic aspect ratio corrected' 1.33(3).

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 14, 2023

scaling the framebuffer to old 1280x1024 displays

What if that becomes aspect_ratio_correct == 2? I think most users want non-stretched mode.

@SoDOOManiac
Copy link
Collaborator

SoDOOManiac commented Apr 14, 2023

What if that becomes aspect_ratio_correct == 2? I think most users want non-stretched mode.

IMO the logic of aspect_ratio_correct == 0 is no correction, just scaling the classic framebuffer to the full display as-is, without maintaining any specific pixel aspect ratio.
aspect_ratio_correct == 1 is for scaled pixel height/width = 1.2, and aspect_ratio_correct == 2 corresponds to scaled pixel aspect ratio = 1 for players who want non-distorted automap and perfectly spherical fireballs.

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 14, 2023

This may be logical, but from the user's perspective on a standard 16:9 screen it looks broken. How are users supposed to know about the aspect_ratio_correct == 2?

@SoDOOManiac
Copy link
Collaborator

SoDOOManiac commented Apr 14, 2023

How are users supposed to know about the aspect_ratio_correct == 2?

That should probably be documented in the readme :)

Anyway, to change aspect_ratio_correct in Crispy you have to edit crispy-doom.cfg file, and reading the manual would be logical before changing that.

@fabiangreffrath
Copy link
Owner

I think aspect_ratio_correct with values 0 and 1 behave exactly the same as in Chocolate Doom. We just added value 2 for people who want non- corrected fixed 1:1 ratio.

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 14, 2023

I think aspect_ratio_correct with values 0 and 1 behave exactly the same as in Chocolate Doom.

In the current master - yes. But in version 3.0 (which people use) aspect_ratio_correct == 0 works differently, like in this PR or in Crispy's aspect_ratio_correct == 2

Anyway, to change aspect_ratio_correct in Crispy you have to edit crispy-doom.cfg file, and reading the manual would be logical before changing that.

Let's face it - no one reads the manual. I think the aspect_ratio_correct in the config file is self explanatory.

@SoDOOManiac
Copy link
Collaborator

In the current master - yes. But in version 3.0 (which people use) aspect_ratio_correct == 0 works differently, like in this PR or in Crispy's aspect_ratio_correct == 2

Yes, in v3.0.1 (which is 3.0.0 with just one critical security exploit fixed IIRC) aspect_ratio_correct == 0 gives square pixels.
But is was changed later for some reason, I deem, and probably that should be discussed with Chocolate team.

@fabiangreffrath
Copy link
Owner

fabiangreffrath commented Apr 17, 2023

In the current master - yes. But in version 3.0 (which people use) aspect_ratio_correct == 0 works differently, like in this PR or in Crispy's aspect_ratio_correct == 2

But Crispy always builds upon the latest Choco master. So, if we change it back to Choco 3.0 state now and Choco 3.1 gets released in 2030 we would still carry the behaviour of the outdated 3.0 release, and not version 3.1 that everybody uses. 😉

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 17, 2023

It's this change: chocolate-doom#1002 I contend that this new behavior should not be the default, I have a user request that it is broken and I myself was of the same opinion.

@rfomin
Copy link
Collaborator Author

rfomin commented Apr 17, 2023

If you disable aspect ratio correction on a standard 16x9 monitor, you end up with an extremely stretched image - who needs that? As an alternative, I suggest adding widescreen support for the "square pixels" case.

@fabiangreffrath
Copy link
Owner

If you disable aspect ratio correction on a standard 16x9 monitor, you end up with an extremely stretched image - who needs that? As an alternative, I suggest adding widescreen support for the "square pixels" case.

Honestly, I don't know. I was never convinced of this idea, that's why I introduced the aspect_ratio_correct == 2 solution in Crispy. If you could move this discussion back to Choco (with a PR to revert the commit in question, or add the aspect_ratio_correct == 2 solution from Crispy) I'd be thankful.

@SoDOOManiac
Copy link
Collaborator

SoDOOManiac commented Apr 18, 2023

I suggest adding widescreen support for the "square pixels" case.

I voted against removing it in the first place :)
This line was without == 1 before:

if (crispy->widescreen && aspect_ratio_correct == 1)

There's even no need to change the comment, because the phrase 'widescreen makes no sense without aspect ratio correction' implies that for square pixels aspect ratio correction is in effect, just enforcing a different pixel aspect ratio value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants