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.

Sign up for GitHub

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

Jump to bottom

Client Settings Revamp with Categories. #950

Merged
merged 2 commits into from Oct 17, 2021
Merged

Client Settings Revamp with Categories. #950

merged 2 commits into from Oct 17, 2021

Conversation

Arufonsu
Copy link
Contributor

@Arufonsu Arufonsu commented Oct 12, 2021 •
edited

About this Pull Request.

  • Renamed "Options" to "Settings" in order to keep consistency.

  • Added Tabs in order to categorize the client settings.

  • "Tabs" are technically Buttons, but we will refer to them as "Tabs" for now, considering what they actually do, visually speaking.

  • Game Settings has it's own tab.
    (it is empty for now)

  • Video Settings has it's own tab.

  • Audio Settings has it's own tab.

  • Keybinding Settings (AKA: "Controls") has it's own tab.

  • Apply button behaves as it usually does, but now saves the Keybinding Settings as well.

  • When accessing the Settings Window from the Main or Escape Menus, Apply will save and take you back there.

  • When accessing the Settings Window from the Main or Escape Menus, Cancel will take you back there without saving.

  • When accessing the Settings Window by pressing a configured key, Apply will save and take you back to game.

  • When accessing the Settings Window by pressing a configured key, Cancel will take you back to game without saving.

Preview.

imagen

imagen

imagen

About the required resources for this PR

  • "settings_window.png" replaces "options_horizontal.png".
  • "SettingsWindow.json" replaces "OptionsWindow.json".
  • "options_dropdown.png" renamed to "settings_dropdown.png"
  • Changes to EscapeMenu.json were made due to the "options" renaming.
  • Changes to MenuWindow.json were made due to the "options" renaming.

(Updated 16-10-2021) Download 'resources.zip' with the required files for this PR
Added the sprites for tabs just in case now, they should be present in the latest asset pack since the addition of the chatbox tabs.
imagen

NOTE: This PR originally included the 3 new settings for entities info visibility, decided to exclude those for now, because they may still require some discussion and attention in a different PR afterwards, so expect more resource updates on this one !

@Arufonsu Arufonsu force-pushed the ClientSettingsRevamp branch 3 times, most recently from 0920f1e to e15bac4 Compare October 16, 2021 07:00
@Arufonsu
Copy link
Contributor Author

  • Updated the pull request description.
  • Updated the pull request preview and resources.
  • Reverted the "back" button to "cancel", as previously discussed in discord with @Cheshire92

Cheshire92
Cheshire92 suggested changes Oct 16, 2021
View reviewed changes
Copy link
Contributor

@Cheshire92 Cheshire92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not getting the tab graphics set up correctly when I download your resource zip?

Image

Intersect.Client/Interface/Shared/SettingsWindow.cs Outdated Show resolved Hide resolved
@Cheshire92
Copy link
Contributor

Also just noticed that Cancel does not return to the escape menu in-game.

@Arufonsu
Copy link
Contributor Author

Arufonsu commented Oct 16, 2021 •
edited

I'm also not getting the tab graphics set up correctly when I download your resource zip?

Image

Do you have the latest resources that include the tabs for the chatboxes?
They are the ones currently being used for the settings tabs, they don't really require any changes.
imagen

Edit: just re-uploaded a zip with them included, just in case.

@Arufonsu
Copy link
Contributor Author

Arufonsu commented Oct 16, 2021 •
edited

Also just noticed that Cancel does not return to the escape menu in-game.

I misunderstood last night's chat then, i though that the original behaviour was expected for the cancel button.
i will see what i can do to make it return to the escape menu while in game again, and to make it hide everything when opened by a configured key as well.

Edit: Might follow the same logic with the Apply button, let me know if you like the final output ~

@Cheshire92
Copy link
Contributor

Ah. I meant that there shouldn't be a difference in labels for the button, it should always be called cancel.. Not randomly cancel, back or exit. :P

However, it should still contextually behave properly.

@Arufonsu
Copy link
Contributor Author

Arufonsu commented Oct 16, 2021 •
edited

  • Applied review changes.
  • Updated Resources (re-aligned some settings, renamed some typos, added the sprites used by tabs just in case).

@Arufonsu Arufonsu force-pushed the ClientSettingsRevamp branch 3 times, most recently from 11bc081 to 24ffbf5 Compare October 16, 2021 18:44
@Arufonsu
Client Settings Revamp with Categories.
7b7fe44 
* Renamed "Options" to "Settings" in order to keep consistency.
* Added Tabs in order to categorize the client settings.
* Game Settings has it's own tab for upcoming settings.
* Video Settings has it's own tab.
* Audio Settings has it's own tab.
* Keybinding Settings (AKA: "Controls") has it's own tab.
* Apply button behaves as it usually does, but now saves the Keybinding Settings as well.
* Renamed the Client strings within the modified structs to comply with the 'Pascal Case' format.

(Cheshire92's Review Changes)
* Added the Json property 'NullValueHandling.Ignore' to the the Client strings within the modified structs.

(Cheshire92's 2nd Review Changes)
* Cleaned up a the code a bit and fixed some typos.
* Turned IsVisible() into a lambda expression.
* Properly assigned a reference to Escape Menu.
* Both, Apply and Cancel button should behave properly now.
* When accessing the Settings Window from the Main or Escape Menus, Apply will save and take you back there.
* When accessing the Settings Window from the Main or Escape Menus, Cancel will take you back there without saving.
* When accessing the Settings Window by pressing a configured key, Apply will save and take you back to game.
* When accessing the Settings Window by pressing a configured key, Cancel will take you back to game without saving.
@Cheshire92
Copy link
Contributor

Cheshire92 commented Oct 17, 2021 •
edited

It's honestly really annoying that you keep force pushing stuff as you're forcing me to re-review the entire thing and not just the changes since I can't tell what has or hasn't been changed like this. Could you please stop doing that?

We squash it down to a single commit when it gets merged so there's no reason to do this.

@Cheshire92
Copy link
Contributor

Cheshire92 commented Oct 17, 2021 •
edited

You've also broken my local version of this now and making me jump through all sorts of hoops to fix it. Well done.
Please don't ever do that again.

In case someone else runs into this, the git command to fix it is: git pull --rebase

Cheshire92 added a commit to AscensionGameDev/Intersect-Assets that referenced this pull request Oct 17, 2021
@Cheshire92
Revamped settings menu (PR #950)
f7be71e 
AscensionGameDev/Intersect-Engine#950
@Cheshire92 Cheshire92 mentioned this pull request Oct 17, 2021
Closed
Cheshire92 added a commit to AscensionGameDev/Intersect-Assets that referenced this pull request Oct 17, 2021
@Cheshire92
Revamped settings menu (PR #950)
e04bc82 
AscensionGameDev/Intersect-Engine#950
Cheshire92 added a commit to AscensionGameDev/Intersect-Assets that referenced this pull request Oct 17, 2021
@Cheshire92
Revamped settings menu (PR #950)
6e5316c 
AscensionGameDev/Intersect-Engine#950
@Cheshire92
Copy link
Contributor

I've just removed repeated code that really didn't need to be duplicated.
Pushed the resources to the development branch of the asset repo as well. :)

@Cheshire92 Cheshire92 merged commit 31bddcf into AscensionGameDev:development Oct 17, 2021
@Arufonsu
Copy link
Contributor Author

It's honestly really annoying that you keep force pushing stuff as you're forcing me to re-review the entire thing and not just the changes since I can't tell what has or hasn't been changed like this. Could you please stop doing that?

Oh.. my bad, didn't knew that you were working with a local repo, i just wanted the PR to be more in shape and less messy with small commits, as i was fixing small undesired things around.

I didn't really pictured how annoying that could of been to be honest :( sorry, won't happen again.

Anyway.. thanks a lot for pushing and helping along with this PR, i hope that the idea and changes end up being useful for everyone when it comes to add new client settings :D !

Regards ~

@Arufonsu Arufonsu deleted the ClientSettingsRevamp branch October 17, 2021 18:55
@Arufonsu Arufonsu mentioned this pull request Oct 17, 2021
Merged
@Arufonsu Arufonsu mentioned this pull request Oct 28, 2021
Open
@Arufonsu Arufonsu mentioned this pull request Feb 12, 2022
Merged
@Arufonsu Arufonsu added the enhancement Minor feature addition or quality of life change label Mar 19, 2022
AlexVild pushed a commit to AlexVild/Middle-Ages-Online-Source that referenced this pull request Mar 22, 2022
@Arufonsu @Cheshire92 @AlexVild
Client Settings Revamp with Categories. (AscensionGameDev#950)
31bc63b 
* Client Settings Revamp with Categories.

* Renamed "Options" to "Settings" in order to keep consistency.
* Added Tabs in order to categorize the client settings.
* Game Settings has it's own tab for upcoming settings.
* Video Settings has it's own tab.
* Audio Settings has it's own tab.
* Keybinding Settings (AKA: "Controls") has it's own tab.
* Apply button behaves as it usually does, but now saves the Keybinding Settings as well.
* Renamed the Client strings within the modified structs to comply with the 'Pascal Case' format.

(Cheshire92's Review Changes)
* Added the Json property 'NullValueHandling.Ignore' to the the Client strings within the modified structs.

(Cheshire92's 2nd Review Changes)
* Cleaned up a the code a bit and fixed some typos.
* Turned IsVisible() into a lambda expression.
* Properly assigned a reference to Escape Menu.
* Both, Apply and Cancel button should behave properly now.
* When accessing the Settings Window from the Main or Escape Menus, Apply will save and take you back there.
* When accessing the Settings Window from the Main or Escape Menus, Cancel will take you back there without saving.
* When accessing the Settings Window by pressing a configured key, Apply will save and take you back to game.
* When accessing the Settings Window by pressing a configured key, Cancel will take you back to game without saving.

* Clean up repeated code

Co-authored-by: Cheshire <azurebeats@hotmail.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cheshire92 Cheshire92 Cheshire92 approved these changes

Assignees
No one assigned
Labels
enhancement Minor feature addition or quality of life change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Arufonsu @Cheshire92