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

Gui Overhaul, Part 1 #229

Merged
merged 6 commits into from Oct 27, 2019
Merged

Gui Overhaul, Part 1 #229

merged 6 commits into from Oct 27, 2019

Conversation

@KrahJohlito
Copy link
Contributor

KrahJohlito commented Oct 19, 2019

Pull Request checklist

Note: these are not necessarily requirements

  • I reformatted the code with clang-format
  • I checked to make sure my submission worked
  • I am the author of submission or have permission from the original author
  • Requires update of the PS2SDK
  • Requires update of the gsKit
  • Others (please specify below)

Pull Request description

- Move game settings into a submenu - Game Menu.
This was quite a big job and resulted in a lot of code changes, mainly from moving the Per-Game Gui related functions into their own source file...(from gui.c into guigame.c).. most of the moved functions themselves however remain the same/similar.

- Move rename and delete into Game Menu & map info to square
The rename and delete functions from the Main page have been moved into the Game Menu, they still require Write Operations to be enabled to be functional. This does 2 things, fixes hint strings on Main being rendered off screen in certain languages and makes room for the Info page to be mapped to Square from Main, so no more being forced to see the Info page before booting a game.

- Hint Strings
Hint strings have unified x and y positions, Themes now have the option of 'aligned=1' to center align hint strings for Main and Info pages.

- Bug Fixes
A couple small bug fixes, commits are self explanatory.

Thanks to @Tupakaveli for testing, suggestions/ideas and helping with overall structure. Team effort.

Test Build
https://www.sendspace.com/file/qs8b8c

@KrahJohlito KrahJohlito changed the title Gui Overhaul Gui Overhaul, Part 1 Oct 19, 2019
@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 19, 2019

That are some nice improvements and IMO a HUGE improvement altogether!

I can't wait for it being merged!

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 19, 2019

Here is how new game menu will look like:
https://i.postimg.cc/wT9Rdb42/new-new.png

Here is how it looks like previously (custom theme):
https://i.postimg.cc/yYrRXynT/preview-17-by-shaolinassassin.png

What about combine both ideas:
https://i.postimg.cc/P5WtrtL8/my-plan.png

Or alternatively put into Game Settings -> (Custom ELF and Game ID):
https://i.postimg.cc/BvWfFRFf/my-plan2.png

Why I want to keep compatibility modes at all cost almost on "surface"?
Because it is the first thing to do\enable when game have any problem(s).
After enabling one of compatibility mode I just hit "Test" button and try if now everything is fine.
When I'll have to enter another sub-menu for it,
sometimes I may be not sure if I enable it (e.g. mode 2).

When everything is fine I can test other things like cheats which can also break compatibility,
but there are not mandatory (game will runs without them), so they can be in separate menu.

It is only my 5 cents, I may like new ideas in future...
I don't know maybe I've got used to some things...

Anyway...

Best regards and thanks for other fixes.

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 19, 2019

Well... If you wouldn't remember, if you activated a function, why not simply re-entering the sub-menu and take a look at it again?

Technically, it could also be the new menu + an on-screen-button or field (within the sub-menu which has the compatibility-modes), which instead of 'saves (the settings)' rather starts the games with the settings, which are about to be tested!

That way we could have the new menu which is not so heavily cluttered and has room for even more additions + the 'inconvenience' you have mentioned should not be existent anymore! ;)

Edit: That's part of the reason, why I suggested 'Test' being an OSD-Button (also in the main-menu) [and I think SQUARE would be appropriate]!

@Tupakaveli

This comment has been minimized.

Copy link
Contributor

Tupakaveli commented Oct 19, 2019

The whole point of the changes is to get away from having everything crammed into a dialog window.

So that it's an actual menu with screen handler target and transition.

@KrahJohlito

This comment has been minimized.

Copy link
Contributor Author

KrahJohlito commented Oct 19, 2019

The reason it looks the way it does currently is because it’s a dialog window being rendered on top of main, hence there is no actual transition... It just appears (causing graphic glitches in HiRes vModes).

Unlike the test build switching to Game Menu transitions just like every other submenu (info & main menu).. I’m not sure it’s possible to retain that and use UI Items outside of dialog windows eg UI_BOOLs Which are things like those Mode ON/OFF switches...

I think the way it is done in the PR is technically more ‘correct’ from a GUI point of view but maybe that’s just me.

I would love to hear from some other people too if they have a chance (even if they don’t agree, I would like to hear their opinions on the changes since if affects some of their existing code).

@rickgaiser @sp193 @BatRastard @belek666

Best regards

@BatRastard

This comment has been minimized.

Copy link
Contributor

BatRastard commented Oct 19, 2019

Fan-freakin'-tastic! :D

Our GUI was something I always wanted to tackle way back when I got involved with beta testing the Ar Tonelico 2 retranslation efforts. Alas, real life crap (i.e. a slip and fall accident = 1year concussion syndrome + neuropathy in my neck that triggers vertigo + my mother's death + self-sufficiency) happened between September 2014 and January 2016 that forced me into retirement. Nonetheless, the Per Game GUI was a big pain in the ass for the reasons KrahJohlito said above which was also compounded on getting the right alignment, separators, breaks, font size, etc. without having either the functions themselves or the help text rendering off screen. I wanted soooo badly to purge and replace it all using retooled and consolidated Main Menu code justified dead center - exactly like the snapshot. Excellent work! It needed to be done for a heck of a long time ... and it finally is! :D 👍

@haudi

This comment has been minimized.

Copy link

haudi commented Oct 20, 2019

Awesome work!

If I'm not mistaken, changing the Game ID is not necessary anymore. All alternative servers are ignoring this today. More space for something else?

In the future this feature could be replaced with a hardcoded function to bypass DNAS completely like DNAS bypass yes/no (this is not the topic here, I know...just my 2 cents :-)

Thanks to all involved.

@carljtc

This comment has been minimized.

Copy link

carljtc commented Oct 20, 2019

If I'm not mistaken, changing the Game ID is not necessary anymore. All alternative servers are ignoring this today. More space for something else?

In the future this feature could be replaced with a hardcoded function to bypass DNAS completely like DNAS bypass yes/no (this is not the topic here, I know...just my 2 cents :-)

Thanks to all involved.

Indeed that ID isnt needed anymore for networked games at least. I dunno about HDD stuff thou.
Would be nice to have a patch to bypass DNAS on the fly. krHacken made a tool for that to create PS2rd patches and ISO patching seems pretty generic among games, some use data integrity tho.

Another great feature would be to set custom i.Link ID into OPL's CDVDMAN so we could mess around with YNC, DNAS-MC.. maybe DNAS-HDD too?

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 20, 2019

Can we keep it to the topic and the CURRENT changes for the time being? ;)

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 20, 2019

So we have 6 votes for definitely yes.
1 to mix things up (not fully yes).

If majority wants to have this new look, than that is just the way it'll probably be.

Thanks once again for support, fresh ideas and keeping this project still alive.
Best regards.

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 20, 2019

Well... We can create a poll on a board as well, but I think it might end up the same.

Nevertheless I would appreciate some input from those who were mentioned and whoever wants to provide his/her personal view.

Edit: It is also possible to have both menus selectable via an OPL-setting, but it just feels kind of redundant IMO.

Edit: @ElPatas1: Did you give it a try? What's your opinion?

Edit: @J013k: I still kind of like your approach as well, but if there are ever any new functions/settings added, it would become even more filled again.

The new menu is more streamlined with the settings-menu and is technically better IMO. The old game-settings-menu is rather 'flicked together', has no transition and so on.

While we are at features: :P

A game-settings-transition-sfx could be loaded from the folder, where ART is usually stored!
A game-specific (game-)settings-transition! That would be awesome and beside the call to the audsrv-lib with the path, it would only need to 'glue' the right 'SLES' to the path. @KrahJohlito ? :D

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 20, 2019

I really like that we can discuss about changes that will be done.
It doesn't matter that I've a little bit other point of view, it matters that someone can change things,
knows how to do it and most of us likes it.

@Jay-Jay-OPL

This comment has been minimized.

Copy link
Contributor

Jay-Jay-OPL commented Oct 20, 2019

I really like this new changes being proposed, I've often felt that Game Settings page was a bit too cluttered.

The only thing I'd like to propose is to add a few horizontal lines to split up some parts. (See the screenshot below)

OPL_Game_Settings-NEW

This way the Settings and Custom pages for each entry could be altogether in a group (within two horizontal lines (top and bottom).

Also, (thinking ahead) I think the "Remove All Settings" should NOT be near the other entries (where you open and fiddle with the settings). I think it should be dead last (see my screenshot above). -- This way less troublesome for some newbies that may accidentally select it (just to browse what's inside) -- for it to delete their customized settings for their game -- without much pre-warning.

Plus if there was a way for that operation to not be so destructive on the first selection, perhaps a pop-up that would say, "Are you sure you want to delete your Settings?!" Press X to Delete or O to Exit and return to the Game Settings Menu.

@Jay-Jay-OPL

This comment has been minimized.

Copy link
Contributor

Jay-Jay-OPL commented Oct 20, 2019

  • Move rename and delete into Game Menu & map info to square
    The rename and delete functions from the Main page have been moved into the Game Menu, they still require Write Operations to be enabled to be functional. This does 2 things, fixes hint strings on Main being rendered off screen in certain languages and makes room for the Info page to be mapped to Square from Main, so no more being forced to see the Info page before booting a game.

As for the above changes, I think if you guys could make it an option under Display Settings or next to the INFO page setting, if they prefer the old or new way. It would be best.

Plus moving the Rename / Delete functions to the Game Settings is great, but it needs to be structured better -- I can later offer another screenshot for it's placement, but I think it would fit right before or after the "Remove All Settings" option on my Screenshot provided above in my earlier reply...

New Screenshot:

OPL_Game_Settings-NEW-2

@ElPatas1

This comment has been minimized.

Copy link
Collaborator

ElPatas1 commented Oct 20, 2019

Hello,

mm...starting to seriously change the GUI maybe is better to open a thread in
psxplace and discuss all the changes, ideas, and test each change before
opening a pull request.

Personally i have to try this change with the custom translation when
i have free time this next week.

Best regards.

@KrahJohlito

This comment has been minimized.

Copy link
Contributor Author

KrahJohlito commented Oct 21, 2019

As for the above changes, I think if you guys could make it an option under Display Settings or next to the INFO page setting, if they prefer the old or new way. It would be best.

I personally won’t be doing this, I think it is an unnecessary option.. We tell the users what buttons do what they don’t need to decide (other than the cross/circle select switch which is understandable for region reasons).

As for the other suggestions, I did try lines in an earlier local build and it ended up being diagonal.. I don’t know if this is due to HSYNC updating or whether I was accidentally incrementing the y pos at some point without realising but I haven’t looked at it since.

I have local changes for another PR later to switch between global and per-game settings during runtime, so adding a global config for game settings and being able to switch between the two...(Supporting GSM, Cheats & PADEMU) it works quite well but that’s for later.. anyway in that build Remove All Settings is a pop up window with multiple options to remove per game, global, both pregame and global or cancel.

@ElPatas1
Please consider this current PR as is, If you need time to test there is no problem. These changes were made for the Ifcaro repository and if they meet your approval feel free to merge when you are ready. As for further discussion or suggestions for later changes in a follow up PR they should indeed take place in a forum.

Thank you
Best regards.

@Tupakaveli

This comment has been minimized.

Copy link
Contributor

Tupakaveli commented Oct 21, 2019

@Jay-Jay-OPL
I'm not trying to be rude here, but since you have your own fork, all these suggestions of yours could be implemented on your end.

Best regards.

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 21, 2019

Well... The line-separations are not a bad idea for grouping.

That's something for the others to poll/vote on. Your choice guys&girls! :)

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 21, 2019

Theoretically discussion about these changes started at forum.
Almost no one "complains" about them:
https://www.psx-place.com/threads/open-ps2-loader-v0-9-3.13415/page-42#post-205985.
That is why this pull request was probably made.

As for the lines to separate options...
Current design looks "pretty raw".
I'm not "saying" it is bad, wrong or something else.

If these lines can be somehow added it'll polish up a little bit this concept.

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 21, 2019

Regarding 'submenus'...

I suppose, if more functions are added later on (if any), the 'Game-Settings'-Entry might be renamed to 'Compatibility-Modes' and all the other functions moved to 'Advanced Settings'.

On another note: The naming-convention isn't really streamlined, is it?
Q settings, R settings, configure S, configure T, configure U, configure V, etc.

It could be 'X-settings' (X being the word for the setting) on all, but it doesn't need to be.

Possibly something alike:
'Compatibility(-Mode) Settings' (or just 'Compatibility Settings')
'Advanced Settings'
'Cheat Settings'
'Video Settings' (GSM)
'MemoryCard Settings' (VMC)
'Controller Settings' (PADEMU)

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 21, 2019

I think that somewhere TnA mention it, or I may be wrong,
but additional "Test" & "Remove Settings" button next to e.g. "OK"
might be good idea:
https://i.postimg.cc/XJbX30LD/test-button33.png
I know that similar option is at the bottom where all the game settings are,
but it is more comfortable to have it also at the specific settings (e.g. Game Settings, GSM...).

Sometimes instead of "Save Setting" I accidentally click "Remove All Setting"...
What about to assign "Remove All Setting" into square next to Select (X) and Back (O)?
Sometime ago it was in early concept:
https://i.postimg.cc/XvgQcnrw/to.png

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 21, 2019

Yes, I mentioned that before! ;)

Either a new 'field' like you have shown in your picture, or a new 'on-screen button' (like 'Select' and 'Back', probably using SQARE)! :)

This would eliminate the inconvenience and 'non certainty', you have mentioned before.

I think a similar full-screen-request (like you have shown in the picture on psx-place.com) could be used as well somewhere there...

@Tupakaveli

This comment has been minimized.

Copy link
Contributor

Tupakaveli commented Oct 21, 2019

If you test the global settings test build on psx-place you'll see that Remove Settings has already been changed.

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 21, 2019

I'm not sure what you mean?
Here is an image from this PR:
https://i.postimg.cc/W4vYGwXf/1.png
Here is an image from new concept with global and per game settings:
https://i.postimg.cc/FRFPqsty/2.png

I was thinking about it:
https://i.postimg.cc/6q8TMRYD/3.png

@Tupakaveli

This comment has been minimized.

Copy link
Contributor

Tupakaveli commented Oct 21, 2019

I mean in that build you are prompted before removing settings so you can't "accidentally" remove all settings.

@J013k

This comment has been minimized.

Copy link

J013k commented Oct 21, 2019

Damn it, another confusion I've made...
Sorry.
I mean "Save Changes" and "Remove All Settings' are so close that to each other,
that sometimes I accidentally press it. I know it sound stupid...
but maybe a suggestion to put "Remove All Settings"
at the end or give this option "square" might not be such a bad idea!?

@Tupakaveli

This comment has been minimized.

Copy link
Contributor

Tupakaveli commented Oct 21, 2019

I think having it as the bottom menu item is not a bad idea, it was that way in an earlier build. It was only moved because I didn't like the way it looked.

@TnA-Plastic

This comment has been minimized.

Copy link

TnA-Plastic commented Oct 21, 2019

Yes, @J013k!
You got it right in your last picture! :)

That's what I meant.
But since it (remove settings) has a 'request-window' now it's alright the way it is proposed/done in the global settings test-build (part 2-build).

It is not 'needed' because of 'accidentially removing the settings', but it would still shorten the list!

@ElPatas1

This comment has been minimized.

Copy link
Collaborator

ElPatas1 commented Oct 26, 2019

@KrahJohlito, i tested the changes and is great.

But before the merge i noticed that in the test build all the games have set by
default the speed of the internal HDD to MDMA 0 instead UDMA 4.

Please check it and fix this, the default speed is UDMA 4.

Best regards.

@KrahJohlito

This comment has been minimized.

Copy link
Contributor Author

KrahJohlito commented Oct 27, 2019

@ElPatas1

You are correct, thank you for spotting this.. I have committed the fix and updated the test build in the description.

Best regards

@ElPatas1 ElPatas1 merged commit 81a94ed into ifcaro:master Oct 27, 2019
@ElPatas1

This comment has been minimized.

Copy link
Collaborator

ElPatas1 commented Oct 27, 2019

Hello,

merged, thank you very much for all the work and the people involved.

Best regards.

@Jay-Jay-OPL

This comment has been minimized.

Copy link
Contributor

Jay-Jay-OPL commented Nov 11, 2019

Just wanted to report that somewhere within this PR, a bug got introduced when MODES 7+8 were removed from the GUI (i.e. OPL_1399_ifcaro_beta).

If we enable MODE 6 (or any modes combo as long as 6 is enabled) using OPL's Game Settings page it will automatically enable MODES 7+8 once we press Save Changes in the Game Menu.

So instead of showing the following line (within code brackets below) on the game's CFG:
$Compatibility=32 (i.e. MODE 6)

We see this instead in the Game's CFG file:
$Compatibility=224 (i.e. MODES 6+7+8)

It doesn't seem to get triggered with any other mode (but that needs more testing), so far MODE 6 seems to be the trigger.

And you can only see this by either looking at the game's CFG or load an older OPL version (i.e. OPL 1392_ifcaro_beta) when those modes still appeared on the page.

So this affects the following ifcaro beta public builds:

  • OPL_1406_ifcaro_beta (PR: #234 )
  • OPL_1399_ifcaro_beta (PR: #229 )

We may also need a BAT to clean up that error for game's CFG files -- that's if users have already gone through a lot of their game collections and had MODE 6 enabled when saving their CFG settings during the attempt to set "Global" and "Per Game" changes for their games. They may all have MODES 7+8 enabled by mistake -- thus having wrong MODES settings for their games.

I hope this helps...

@KrahJohlito

This comment has been minimized.

Copy link
Contributor Author

KrahJohlito commented Nov 11, 2019

Having 7 & 8 enabled shouldn’t actually do anything since they were removed, it’s probably cause I just didn’t bother to render them in the GUI but left the mode count alone. Should be an easy fix I’ll have a look at it when I can get on pc and have it in the next PR.

@Jay-Jay-OPL

This comment has been minimized.

Copy link
Contributor

Jay-Jay-OPL commented Nov 11, 2019

That was going to be my follow-up question, whether it actually causes an issue down the line or simply later when we attempt to share CFGs back and forth, will the MODES 7/8 haunt us later on (e.g. bad game reports), because some adjusted those settings during this bug.

@Jay-Jay-OPL

This comment has been minimized.

Copy link
Contributor

Jay-Jay-OPL commented Dec 4, 2019

@KrahJohlito since it's been a while. I was wondering if you still plan to push that "bug fix" about the 7/8 modes bug being incorrectly enabled when users have mode 6 enabled in OPL?

I see that you've done the fix on your repo a while back: KrahJohlito@dc18df2

And well, I'm sure if you were to push "only" that fix to the ifcaro repo, it would get accepted rather quickly since bug fixes that were introduced in prior commits (PRs) are usually first priority.

If you still plan to wait to push it on a bigger commit, could I have your permission to cherry pick that bug fix from your repo? Because we do need it. -- Of course, you will get full credit for that fix...

Let me know?

@KrahJohlito

This comment has been minimized.

Copy link
Contributor Author

KrahJohlito commented Dec 4, 2019

Real life got in the way, I don’t have a computer set up currently as I’m still in the process of moving house.. if you wanna cherry pick it you can, but yea there will be another Pr coming when everything is sorted.

The reason that fixes it is, the loop that saves compat modes iterates x number of times based on the compat mode count and since it’s currently doing it 2 more times than there are modes selectable in the GUI if mode 6 is enabled the next two loop iterations are also enabled since their values are never checked and overwritten (because they aren’t in the GUI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.