feature: Add aspect ratio info to resolution selection dialog by Osmodium · Pull Request #6587 · ihhub/fheroes2

Osmodium · 2023-02-06T23:58:07Z

resolves #6586

src/engine/math_base.h

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_resolution.cpp

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_resolution.cpp

oleg-derevenetz · 2023-02-07T00:21:48Z

Hi @Osmodium please remove extra spaces at the end of the line and reorder the includes as suggested by the code style check workflow.

Osmodium · 2023-02-07T10:23:19Z

I kind of wanted to add the ratios as two new fields on ResolutionInfo but since it needs to be computed based on width and height it would require for the constructor to calculate it, or where it is instantiated to calculate it. Don't know if it would even be a concern since the only time we spend resources calculating it, is when the dialog is shown.

oleg-derevenetz · 2023-02-07T10:47:50Z

If this ratio is only needed in one place for illustration purpose only, then there is no need to store it in the ResolutionInfo. It is better to calculate it in-place.

ihhub

Hi @Osmodium , added few comments in this pull request. Could you please take a look when you have time?

src/fheroes2/dialog/dialog_resolution.cpp

ihhub · 2023-02-12T13:54:39Z

src/fheroes2/dialog/dialog_resolution.cpp

+            const int32_t gcd = std::gcd( resolution.width, resolution.height );
+            const auto aspectRatioString = std::to_string( resolution.width / gcd ) + " : " + std::to_string( resolution.height / gcd );


What if the aspect ratio is more than 99 / 99? I see some weird aspect ratios:

We shouldn't do this. I suggest add logic to verify if division by gcd doesn't make values normally looking then increased it to the value when they do.

1366x768 was created as an approximated 16:9 aspect ratio:
https://superuser.com/questions/946086/why-does-1366x768-resolution-exist

I suppose this is the same for 1360x768.

I have included code to not show the aspect ratio if it is above 99, however this was all just suggestions 😃.

The image below shows, not committed code that gets the closest aspect ratio that looks "good":

Is this wished?

src/fheroes2/dialog/dialog_resolution.cpp

Osmodium · 2023-02-12T18:26:07Z

@ihhub, so I just posted images of potential layouts in the issue to find out which direction to go.
I mostly like this one but not showing the x1 (only showing scale when above 1):

Osmodium · 2023-02-13T10:09:57Z

@ihhub I might even like this layout better

ihhub · 2023-02-13T16:23:50Z

Hi @Osmodium , since most of readers in the world read text from left to right we have to put the most important information at the left and less important on the right. Ratio factor is less important information I believe. What do you think?

Osmodium · 2023-02-13T16:41:36Z

Hi @Osmodium , since most of readers in the world read text from left to right we have to put the most important information at the left and less important on the right. Ratio factor is less important information I believe. What do you think?

Hi @ihhub
I certainly see your point. I can get behind this format too 😄

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_resolution.cpp

ihhub · 2023-02-14T16:17:30Z

Hi @Osmodium , think about the design from a perspective of a new player. The player will see something like x2 and x3 on the left corner and immediately rises a question "What is this?" While putting it just besides the resolution intuitively is being understandable as a scale factor.

Osmodium · 2023-02-14T16:23:02Z

Hi @Osmodium , think about the design from a perspective of a new player. The player will see something like x2 and x3 on the left corner and immediately rises a question "What is this?" While putting it just besides the resolution intuitively is being understandable as a scale factor.

Hi @ihhub.
This was why I had placed it on the right since it would make more sense it being a scale factor, however I'm not sure all players, new or otherwise will get what a (x2) means intuitively.
I tried having it in parenthesies (image is for demonstration purposes)

But I feel like this looks too cramped.
A thing we could try it to have a header for each column that indicates the information in that column?
EDIT: Hmm adding a column might prove a bit more difficult since the dialog is one big sprite :)

zenseii

LGTM

Osmodium · 2023-03-09T16:12:28Z

@ihhub Do you have further to this issue? :)

ihhub · 2023-03-09T16:14:03Z

@ihhub Do you have further to this issue? :)

Hi @Osmodium , I will review this pull request again tomorrow. It is on my TODO list. I am also adding @Branikolog to gather his opinion.

Branikolog · 2023-03-09T19:22:30Z

Hi @ihhub and @Osmodium
Sorry, but I'm not really satisfied how this window looks now. It's not so bad though and I can accept it, but I wish it looks more elegant... The aspect ratio feature is good, but I personally cannot propose any good looking integration of this feature into the current window. Maybe we need more global rework of the whole window now?
Also, I'm concerning about the variety of available aspect ratio values.

Could we approximate the first number, which is before the colon ( : ) in such way, so the second number would take more common values, like 3,4,9 or 10? I think it's more demonstrative to have like [15.9 : 9] or [17.7 : 10] rather than [62 : 35].

ihhub

HI @Osmodium , I left several comments in this pull request. Could you please take a look?

I also agree with @Branikolog regarding the visual look of the feature. We should change the dialog to support the feature properly.

src/fheroes2/dialog/dialog_resolution.cpp

ihhub · 2023-03-11T14:56:36Z

src/fheroes2/dialog/dialog_resolution.cpp

+        void ActionListDoubleClick( fheroes2::ResolutionInfo & item, const fheroes2::Point & /*unused*/, int32_t /*unused*/, int32_t /*unused*/ ) override
+        {
+            ActionListDoubleClick( item );
+        }


What is the reason to override this method which calls another overridden method?

I think it was from some other implementation, just forgot to remove it.
e26cde4

Ah I remember, it was a clang error that told me to add it:
https://github.com/ihhub/fheroes2/actions/runs/4610597218/jobs/8149253282?pr=6587
Reintroduced here: b9a2b4a

github-actions bot assigned Osmodium Feb 6, 2023

Osmodium mentioned this pull request Feb 6, 2023

Display aspect ratio in resolution select dialog #6586

Open

1 task

oleg-derevenetz reviewed Feb 7, 2023

View reviewed changes

src/engine/math_base.h Outdated Show resolved Hide resolved

oleg-derevenetz linked an issue Feb 7, 2023 that may be closed by this pull request

Display aspect ratio in resolution select dialog #6586

Open

1 task

oleg-derevenetz added improvement New feature, request or improvement ui UI/GUI related stuff labels Feb 7, 2023

oleg-derevenetz added this to the 1.0.2 milestone Feb 7, 2023

github-actions bot reviewed Feb 7, 2023

View reviewed changes

src/fheroes2/dialog/dialog_resolution.cpp Outdated Show resolved Hide resolved

src/fheroes2/dialog/dialog_resolution.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Feb 7, 2023

View reviewed changes

src/fheroes2/dialog/dialog_resolution.cpp Show resolved Hide resolved

src/fheroes2/dialog/dialog_resolution.cpp Show resolved Hide resolved

Osmodium force-pushed the feature/add-aspect-ratio-info branch 2 times, most recently from 99f3135 to 65c3345 Compare February 11, 2023 19:45

ihhub requested changes Feb 12, 2023

View reviewed changes

Osmodium force-pushed the feature/add-aspect-ratio-info branch from e9840c7 to 9052d6d Compare February 12, 2023 18:37

Osmodium requested review from ihhub and oleg-derevenetz and removed request for ihhub and oleg-derevenetz February 12, 2023 22:43

Osmodium force-pushed the feature/add-aspect-ratio-info branch from 03a8bfb to 590392f Compare February 13, 2023 17:41

github-actions bot reviewed Feb 13, 2023

View reviewed changes

src/fheroes2/dialog/dialog_resolution.cpp Outdated Show resolved Hide resolved

oleg-derevenetz reviewed Feb 13, 2023

View reviewed changes

src/fheroes2/dialog/dialog_resolution.cpp Outdated Show resolved Hide resolved

zenseii requested a review from ihhub March 6, 2023 10:24

zenseii approved these changes Mar 6, 2023

View reviewed changes

ihhub requested a review from Branikolog March 9, 2023 16:14

ihhub requested changes Mar 11, 2023

View reviewed changes

ihhub modified the milestones: 1.0.2, 1.0.3 Mar 12, 2023

Osmodium added 3 commits April 4, 2023 19:27

Remove override method

e26cde4

Reintroduced newline

8f9f8a1

Refactor aspect ratio approximation method

2c9db0c

Osmodium requested a review from ihhub April 4, 2023 17:29

Reintroduced ActionListDoubleClick to satisfy Clang

b9a2b4a

ihhub modified the milestones: 1.0.3, 1.0.4 Apr 11, 2023

ihhub modified the milestones: 1.0.4, 1.0.5 May 13, 2023

ihhub modified the milestones: 1.0.5, 1.0.6 Jun 14, 2023

ihhub modified the milestones: 1.0.6, 1.0.7 Jul 15, 2023

ihhub modified the milestones: 1.0.7, 1.0.8 Aug 14, 2023

ihhub modified the milestones: 1.0.8, 1.0.9 Sep 11, 2023

ihhub modified the milestones: 1.0.9, 1.1.0 Oct 11, 2023

ihhub modified the milestones: 1.1.0, Beyond OG scope Jan 13, 2024

		const int32_t gcd = std::gcd( resolution.width, resolution.height );
		const auto aspectRatioString = std::to_string( resolution.width / gcd ) + " : " + std::to_string( resolution.height / gcd );

Uh oh!

Conversation

Osmodium commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oleg-derevenetz commented Feb 7, 2023

Uh oh!

Osmodium commented Feb 7, 2023

Uh oh!

oleg-derevenetz commented Feb 7, 2023

Uh oh!

ihhub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihhub Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

zenseii Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

Osmodium Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Osmodium commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Osmodium commented Feb 13, 2023

Uh oh!

ihhub commented Feb 13, 2023

Uh oh!

Osmodium commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ihhub commented Feb 14, 2023

Uh oh!

Osmodium commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zenseii left a comment

Choose a reason for hiding this comment

Uh oh!

Osmodium commented Mar 9, 2023

Uh oh!

ihhub commented Mar 9, 2023

Uh oh!

Branikolog commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihhub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ihhub Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

Osmodium Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

Osmodium commented Feb 6, 2023 •

edited

Loading

Osmodium Feb 12, 2023 •

edited

Loading

Osmodium commented Feb 12, 2023 •

edited

Loading

Osmodium commented Feb 13, 2023 •

edited

Loading

Osmodium commented Feb 14, 2023 •

edited

Loading

Branikolog commented Mar 9, 2023 •

edited

Loading

Osmodium Apr 4, 2023 •

edited

Loading