Page MenuHomeWildfire Games

Do not hardcode screen ratio in the main menu.
AbandonedPublic

Authored by Stan on Nov 24 2018, 4:41 PM.

Details

Reviewers
elexis
bb
Trac Tickets
#5353
Summary

Agree

Test Plan

Check if it doesn't break anything.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6464
Build 10704: Vulcan BuildJenkins

Event Timeline

Stan created this revision.Nov 24 2018, 4:41 PM
Vulcan added a subscriber: Vulcan.Nov 24 2018, 4:42 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/797/

lyv added a subscriber: lyv.Nov 25 2018, 10:33 AM
lyv added inline comments.
binaries/data/mods/public/gui/pregame/mainmenu.js
93

(Anyone else feel this is a pretty ugly way to show the splashscreen?)

Stan marked 2 inline comments as done.Nov 25 2018, 10:41 AM
Stan added inline comments.
binaries/data/mods/public/gui/pregame/mainmenu.js
93

Feel free to submit a patch :)

Stan can you describe how this number is used? It is not clear to me from a quick read and quick test (and why 1 is better than 1.7777?)

binaries/data/mods/public/gui/pregame/mainmenu.js
93

/me raises hand
(If it isn't just 10 lines of code I'd have proposed to move it to a separate file, also md5sum #4399)
(Reminds me that I also have a mainmenu.xml rewrite somewhere moving that stuff to this file)

Stan marked an inline comment as done.Nov 25 2018, 10:53 AM

Basically this number is multiplied to whatever the height is. So in case it's 1080 you get 1920 if it's 1050 you get 1866 instead of 1680 (Because it's supposed to be a 16:10 ratio) if you give it 768 (for 1024x768 which is the minimun resolution we support) you get 1366. That is not a 4:3 ratio.

elexis added a comment.EditedNov 25 2018, 11:13 AM
In D1680#66674, @Stan wrote:

Basically this number is multiplied to whatever the height is. So in case it's 1080 you get 1920 if it's 1050 you get 1866 instead of 1680 (Because it's supposed to be a 16:10 ratio) if you give it 768 (for 1024x768 which is the minimun resolution we support) you get 1366. That is not a 4:3 ratio.

I understand that it's a multiplication, but what is the purpose of the number?

Edit: rP10042 philip var w = h*16/9; // width of background image

Stan added a comment.Nov 25 2018, 11:32 AM

It's only used to compute the offset of the background. it's passed as an argument to the background to offset it.

Each background defines for each of it's sprites this:

{
    "offset": (time, width) => 0.02 * width * Math.cos(0.05 * time),
    "sprite": "background-kush1-1",
    "tiling": true,
},

Not to mention the computation is really innefficient because it computes it one time per layer.

Just mere guessing, but doesn't that hardcode the ratio of the image rather than the ratio of the screen and tries to make sure that the image is moved into all directions with equal speed?

Stan added a comment.Nov 25 2018, 11:57 AM

The screen height is dependent on the window size.

Did you test it on really wide monitors/windows?

Stan added a comment.Nov 25 2018, 12:22 PM

I tried it by stretching the window to give it an 21/9 ratio.

Stan added a comment.May 6 2019, 1:02 AM

@bb, @vladislavbelov Anything else you want me to fix ?

The problem with the current code is that at no time we recover the real size of the backgrounds.
Depending on the window size the tiling backgrounds are more or less fully displayed.

Stan added a comment.May 20 2019, 11:20 AM

The problem with the current code is that at no time we recover the real size of the backgrounds.
Depending on the window size the tiling backgrounds are more or less fully displayed.

Well it shouldn't matter as long as they re-size accordingly, should it ?

Test is better than words.

Sets a window size of 1280x1024 by example and uses the Kushite background.
At the end of the scroll to the right, the right side of the city is still not displayed.

elexis added a comment.Sep 6 2019, 3:44 AM

Function was moved to BackgroundHandler.js.

For me h * 16/9 = 1818.6666666666667 and screen.right - screen.left = 1916.

One can set different w values to see how it impacts the animation.

For example w=2 = no background scrolling, everything looks fine.
and w=2000000 super fast scrolling, the background is repeating, otherwise fine.
Fine as in the images are displayed in proportion.

If there are some elements of the picture that should be displayed but aren't, like Polakrity said, then this may be accounted for in the background file as well.

I suppose it would be good to test tiling backgrounds, somehow, or become sure that it works for them as well in theory.

binaries/data/mods/public/gui/pregame/mainmenu.js
93

In D2390 you wrote:

This is still not a good idea to hardcode that (though it's not related to that patch directly)

Because this should be computed and/or cached and updated on window resize.
Because the minimum resolution we claim to support is 1024/768 and that's a 4:3 ratio

But I don't see why "this should be computed and/or cached and updated on window resize" nor why this has anything to do with the minimum resolution or the screen ratio at all.
If I use a 4:3 monitor should the icons on the desktop use 4:3 ratio too, and for 16:9 they should use 16:9 ratio?

You can put literally every value in there and it will work correctly without showing any black bars, aspect ratio of 0.0001 or 1000 or without changing the aspect ratio of the image.
What it does change the is the width of the backdrop scrolled, and it repeats the image with tile=true and it does not repeat it with tile=false (in which case the author must ensure that the formula works in such a way to always render something at the background).

Stan abandoned this revision.Jan 13 2020, 10:14 AM

This would now need a rebase, and it doesn't seem to have much of an impact, so meh.