Author Topic: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time  (Read 42282 times)

0 Members and 1 Guest are viewing this topic.

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Thanks for checking the patch.  I've put some comments in the pull request - I'll also copy them here:

The only problem that I noticed is that the if statement at line 1,115 in hudparse.cpp is missing an else statement that calls adjust_base_res(). Because of this, positionless gauges (such as target brackets) that are specified in a HUD table will stretch.
If I understand correctly - you mean add:
Code: [Select]
} else {
  adjust_base_res(base_res);
}

at line 1139?

Another thing: the gauge_load_common() function has a parameter called default_position. It defaults to false, and no calls to this function ever set it to true. What is this parameter for?

This is the one gauge (load_gauge_messages) that uses "default_position" - it's to deal with the magic number on line 4266


EDIT: Oh, and you've got two semicolons at line 5,087 in hudparse.cpp.

Thanks & fixed.

(I'd post a patch, but I can't figure out how to create one. I'm using TortoiseGit, by the way.)

I think TortoiseGit has a "diff" option in its menu, so I figure you mean that you can't get the code that you need to base the diff from?

I don't have much experience with TortoiseGit, but assuming that you've already got the main repo cloned, you can probably do this:
1) setup my fork as a remote (https://github.com/niffiwan/fs2open.github.com)
2) pull (i.e. fetch) the "yarn_hud" branch to a local branch
3) code & send diff
(there's probably other ways to move the commit around without pasting diffs, but I'm not 100% sure on how to do it, probably involves having your own fork on github, a copy of the yarn_hud branch & sending my fork a pull request)

Alternatively, you can just clone my fork and checkout/switch to the yarn_hud branch.  I think this page has a decent explanation:
http://joelabrahamsson.com/remote-branches-with-tortoisegit/

The relevant section is titled "Fetching the remote branch", and the remote branch name to use is remotes/origin/yarn_hud.

If that doesn't help, let me know some details and I'll see how else I can help.  I should note that this is exactly the type of issue that we want to sort out / document properly before we make a switch to git as the primary FSO repository.
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
(I'd post a patch, but I can't figure out how to create one. I'm using TortoiseGit, by the way.)

- Right click on the /code/ folder or, if you have the code folder already open, right click in an empty area of the File Explorer.
- Select "TortoiseGit" from the menu, then "Diff" (which should be somewhere in the middle of the r-click menu)
-- A dialog should pop up showing a diff, or another one to inform you that you haven't chosen a program that will operate on Diff's, TortoiseUDiff shall work fine
- You may copy all text in the diff and paste it in a forum post (encapsulated by the code tags), if the diff is short enough. If it's not, then you'll have to save the diff to a .diff file and upload it somewhere and then link us to it.
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline Yarn

  • 210
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
If I understand correctly - you mean add:
Code: [Select]
} else {
  adjust_base_res(base_res);
}

at line 1139?
Correct.

This is the one gauge (load_gauge_messages) that uses "default_position" - it's to deal with the magic number on line 4266
I see what you mean. (It's actually load_gauge_fixed_messages(), though.) Your code appears to handle this correctly.

- Right click on the /code/ folder or, if you have the code folder already open, right click in an empty area of the File Explorer.
- Select "TortoiseGit" from the menu, then "Diff" (which should be somewhere in the middle of the r-click menu)
-- A dialog should pop up showing a diff, or another one to inform you that you haven't chosen a program that will operate on Diff's, TortoiseUDiff shall work fine
- You may copy all text in the diff and paste it in a forum post (encapsulated by the code tags), if the diff is short enough. If it's not, then you'll have to save the diff to a .diff file and upload it somewhere and then link us to it.
Thanks, this worked! I'm used to TortoiseSVN, so I was trying to use "Create Patch Serial" to make the patch, but it appeared to do nothing. I searched the Internet and found that "Create Patch Serial" required you to commit your changes first (what's the point of making a patch if you've already committed your changes?). Other instructions I found either didn't work or required me to enter a name and e-mail address (why, TortoiseGit?). I would have never guessed to pick "Diff" because, in TortoiseSVN, this option just shows a side-by-side comparison.

I have no idea why the TortoiseGit team decided to deviate so much from the more intuitive way that TortoiseSVN works. I know that Git works differently from SVN and that TortoiseGit had to be adapted to that system, complete with its different terminology, but there's still no obvious reason that they couldn't have made the equivalent of "Create Patch" work the way it does in TortoiseSVN.
"Your fighter is running out of oil.  Please check under the hood and add more if necessary"
--strings.tbl, entry 177

"Freespace is very tired.  It is shutting down to get some rest."
--strings.tbl, entry 178

 

Offline chief1983

  • Still lacks a custom title
  • Moderator
  • 212
  • ⬇️⬆️⬅️⬅️🅰➡️⬇️
    • Minecraft
    • Skype
    • Steam
    • Twitter
    • Fate of the Galaxy
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
The method just described does exist in TortoiseSVN, so possibly they removed the other Create Patch method because it was superfluous, and to trim down the interface a little bit to keep it usable.
Fate of the Galaxy - Now Hiring!  Apply within | Diaspora | SCP Home | Collada Importer for PCS2
Karajorma's 'How to report bugs' | Mantis
#freespace | #scp-swc | #diaspora | #SCP | #hard-light on EsperNet

"You may not sell or otherwise commercially exploit the source or things you created based on the source." -- Excerpt from FSO license, for reference

Nuclear1:  Jesus Christ zack you're a little too hamyurger for HLP right now...
iamzack:  i dont have hamynerge i just want ptatoc hips D:
redsniper:  Platonic hips?!
iamzack:  lays

 

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Thanks for the feedback, I'll add a new commit with those updates (done - see the pull request), then merge it all back into SVN.

... "Create Patch Serial" required you to commit your changes first (what's the point of making a patch if you've already committed your changes?).

That's one profound difference between svn & git (or other DVCS).  When you commit, you're only committing to your local copy of the repository, not to a public repository. So it's quite common to commit a patch to a local branch, then send the diff of that patch off somewhere, either via email/forum post, or push it to a public repo and issue a pull request.
« Last Edit: December 09, 2013, 03:28:19 pm by niffiwan »
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline chief1983

  • Still lacks a custom title
  • Moderator
  • 212
  • ⬇️⬆️⬅️⬅️🅰➡️⬇️
    • Minecraft
    • Skype
    • Steam
    • Twitter
    • Fate of the Galaxy
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Yeah I've recently gotten the hang of using git format-patch and git am at work, since we have a couple repositories with restricted access.  Make your commits locally, email/IM the patch, and someone with push rights applies and pushes the commits to our primary server.
Fate of the Galaxy - Now Hiring!  Apply within | Diaspora | SCP Home | Collada Importer for PCS2
Karajorma's 'How to report bugs' | Mantis
#freespace | #scp-swc | #diaspora | #SCP | #hard-light on EsperNet

"You may not sell or otherwise commercially exploit the source or things you created based on the source." -- Excerpt from FSO license, for reference

Nuclear1:  Jesus Christ zack you're a little too hamyurger for HLP right now...
iamzack:  i dont have hamynerge i just want ptatoc hips D:
redsniper:  Platonic hips?!
iamzack:  lays

 

Offline MachManX

  • 26
  • The Fight Never Ends...This Is A Fact Of Life!
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Well there are a few questionable files that I see.  Why do you have 3 data folders?   You should only have "data" folder.  I dunno why you have a CD.dll file and what that gfw_high does either.  Google search indicates that it is something that comes with PlayonMac, which is used on a Mac, and yet you say it's not.  I mean your specs are PC type.  So why do you have PlayonMac files in that folder?  I can only assume that there is something you're not telling us as the gfw_high file is the proof.
All of the files and folders that you're questioning here is normal for GOG.com installations. Gfw_high.ico is just an icon file for the desktop and start menu shortcuts, and data2, data3, and CD.dll exist so that the original CDs are not required. They have nothing to do with PlayonMac.

Ok, that clears that up.  I'm just questioning his installation because he had that FS2 "ant" file which you said was an old version build.  Makes me wonder if he has a mixture of old and new files in his installation.  I even see .dmg files which are for Mac.  It seems to me like the installation itself is borked.  This is why I recommended starting from scratch again.  Just follow my previous post.

Also I wanted him to do an OpenGL test with one of the programs I mentioned just to see if his system installation isn't causing issues with OpenGL.

Right now I'm wondering how he's configured his desktop primary monitor settings.  Whichever is set to primary will be the one the game is run on, unless there's an option to handle multiple video cards in wxLauncher?
AMD Ryzen 5 3600
B450I GAMING PLUS AC
Geforce GTX 1060 6GB
16GB DDR4-3600
WD SN750 1TB NVME
Samsung 850 EVO 250GB SSD
Corsair HX520W PSU
Cougar QBX Case
NEC V422 42" @ 1080p
Ubuntu 20.04 + Whatever I VM

 

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
And it's committed :)
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline wookieejedi

  • 29
  • Intensify Forward Firepower
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Woot!

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Wiki new HUD.tbl thangs, plz?

Can't make use of them for custom HUDs if you don't document it somewhere.
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline Axem

  • 211
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
So I've been playing around with this awesome stuff a little bit and I have a (hopefully small) request. I like the re-positioning of the hud based on the resolution, but I don't like the scaling of the hud elements. (I really like this, this is ok, but this I also really like.) I like them to be sharp, even if they are smaller in size. I know that's a sort of a personal preference, but maybe there could be an option in like a mod table or hud gauge table to stop the hud element scaling? So it would be like the base hud resolution is always the current window resolution.

(FYI The pic with the table mod was just done with the table in the first post but with the base res changed to (1440,900))

 

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Wiki new HUD.tbl thangs, plz?

Can't make use of them for custom HUDs if you don't document it somewhere.

Done, should just be two new options, origin & offset.
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
On the subject of always having non-scaled/crystal-clear gauges, I think this could be achieved by having adjust_base_res() always set the base res to the current screen res (activated by a global hud_gauges.tbl option).  My only question is, what side effects might this have?  I need to do some tests with odd resolutions, but I'm not sure I can properly test a triple-head style setup.

Also, I was wondering about the situation where a modder specifies only one of Origin: / Offset: for a gauge (e.g. the cause of this ticket).

If only Origin is specified, I thought that it would make most sense to set the Offsets to 0 (I think this situation could be fairly common)
If only Offset is specified, I thought that it would make most sense to leave the Origins as their default values (I think this situation would be fairly uncommon)

Anyone got any thoughts / comments on these points?
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline geo111

  • 21
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
niffiwan,

I'd be glad to do any triple monitor testing you need.

I use three old Dell FP2001 displays, each has a native resolution of 1600x1200. The 3X1 array, with bezel compensation has a resolution of  5076x1200.

I can change the resolution some, or even shut off the bezel compensation if needed.


 

Offline m!m

  • 211
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
On the subject of always having non-scaled/crystal-clear gauges, I think this could be achieved by having adjust_base_res() always set the base res to the current screen res (activated by a global hud_gauges.tbl option).  My only question is, what side effects might this have?  I need to do some tests with odd resolutions, but I'm not sure I can properly test a triple-head style setup.

Also, I was wondering about the situation where a modder specifies only one of Origin: / Offset: for a gauge (e.g. the cause of this ticket).

If only Origin is specified, I thought that it would make most sense to set the Offsets to 0 (I think this situation could be fairly common)
If only Offset is specified, I thought that it would make most sense to leave the Origins as their default values (I think this situation would be fairly uncommon)

Anyone got any thoughts / comments on these points?
Disclaimer: I haven't worked much with HUD modding in general
If I want to position for example the crosshair in the center of the screen, do I have to repeat the offsets every time I add it to my hud table? If I understand your proposal correctly specifying something for Origin will reset the preset offsets to 0 so I will have to specify them in the table for every hud gauge that should not have the top left corner as the origin.
Please correct me if I misunderstood something you said :nod:

 

Offline niffiwan

  • 211
  • Eluder Class
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Thanks you for that, I think I misunderstood the purpose of offsets.  With my proposed change then yes, in order to specify a gauge via its centre position then you'd need to add the offsets manually for each gauge.

However, my original thought process, and the thought process of some others that I discussed this with on IRC was that you specify gauge positions based on the top left corner of the gauge, not the centre.  And thus the confusion :)  I guess that the fact that the Origin: values used in the original patch were 0.0, 0.5 & 1.0 added to my confusion, i.e. the offsets didn't seem to be used to provide the position of the top left of the gauge compared to the centre of the gauge.

So, what is easier for the modder?  Lets use the 1024x768 FS2 centre reticle (gunsight/crosshairs) as an example.

Defaults coded into FSO:
Code: [Select]
+Center Reticle:
Origin:  (0.5, 0.5)
Offset:  (-19, -14)

Example one: move the reticle off centre / ~50 pixels to the left for an Ursa/Rhea like offset firing point, using current code
Code: [Select]
+Center Reticle:
Offset:  (-19, -64)
(assume the modder knows what the default offset is)

Example two: same thing except specify the top left corner position of the gauge and assume the proposed change was added
Code: [Select]
+Center Reticle:
Origin:  (0.487,0.370)

Example three: same as two, except without the proposed change
Code: [Select]
+Center Reticle:
Origin:  (0.487,0.370)
Offset:  (0, 0)

Note, I have not tried these using these examples in game.

Is it simply a case of being easier to specify position via centre (current code), or via top left (proposed code)?

It the best solution just knowledge/documentation? (with a better Assertion message?)
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
I guess it depends on the orientation of the gauge, Top Left works well for gauges that'll be in the top left area, Top Right works well for gauges that'll be in the top right area, and so on.

Reticles, crosshairs, and the like shouldn't need to be offset unless some weirdness with the sizing of the graphic comes into play. I believe these should default to center themselves, favoring the pixel top-left immediately close to the center if a center pixel cannot be calculated. However, I have seen some gun sights that use a post instead of the cross hair, so a center alignment on both the x and y axes would not make sense for these types, and the inverted post likewise.

Maybe if an alignment, or draw direction, was specified, modders could specify a custom gauge like so:
Code: [Select]
+Center Reticle:
Origin:    (0.5, 0.5)
Alignment: (center, center)

+ETS Retail:
Origin: (0.99, 0.66)
Alignment: (right, center)

+Target Monitor:
Origin: (0.1, 0.66)
Alignment: (left, bottom)

What do you think about this?
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline Yarn

  • 210
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Example two: same thing except specify the top left corner position of the gauge and assume the proposed change was added
Code: [Select]
+Center Reticle:
Origin:  (0.487,0.370)

Example three: same as two, except without the proposed change
Code: [Select]
+Center Reticle:
Origin:  (0.487,0.370)
Offset:  (0, 0)

Setting aside the fact that your origin value here is way off for the default reticle, your example here will cause the reticle to go off-center if the game's aspect ratio doesn't match that of the specified base resolution. Do the same with the other gauges and everything can become misaligned. It's almost never a good idea to place the origin where the top-left of the gauge would be in the base resolution. Even placing the origin in the center of the gauge can cause misalignment if the modder isn't careful. I designed the internal HUD the way it is for a reason.

Just remember that the origin parameter alone isn't meant to specify where exactly the gauge is; it's meant to be used with the offset parameter to do that. It's probably a good idea to require an offset to be specified if an origin is also specified.

It looks like the modding community could use some guidelines for origin placement, so I'll make a list of those soon.
"Your fighter is running out of oil.  Please check under the hood and add more if necessary"
--strings.tbl, entry 177

"Freespace is very tired.  It is shutting down to get some rest."
--strings.tbl, entry 178

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
Doh, I misunderstood something somewhere.

Origin is where on the screen the graphic is placed, Offset is the point on the graphic itself that is placed at the Origin. So, alignment is taken care of depending on where you put the offset.

Again, with the crosshair example. If the graphic is a 32x32, then:

Code: [Select]
+Center Reticle:
Origin: (0.5, 0.5)
Offset: (16, 16)

Having a negative offset would mean that the point that's set at the origin is not on the graphic itself, but off to the left and/or above.
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline Yarn

  • 210
Re: [CODE REVIEW] Adaptable HUD, a.k.a. No more HUD stretching, for real this time
You got Offset backwards. Offset specifies how many pixels to add to the point calculated from Origin. Positive numbers move the gauge down and right, while negative numbers move it up and left. It's certainly not "the point on the graphic itself that is placed at the Origin."

The origin is not quite "where on the screen the graphic is placed." Rather, it serves as an "anchor point" for determining how the gauge is positioned if the game's aspect ratio is different from that of the HUD; it can and usually does lie outside the gauge graphic. Also, many gauges in the default HUD share an origin so they aren't spread out more in wider or taller resolutions.

I don't really know how other PC games make non-stretching HUDs that work with multiple aspect ratios. If any have a more intuitive approach, I'd like to hear about them.
"Your fighter is running out of oil.  Please check under the hood and add more if necessary"
--strings.tbl, entry 177

"Freespace is very tired.  It is shutting down to get some rest."
--strings.tbl, entry 178