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

Add simple Game Settings #1281 #1431

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Conversation

Sjerty
Copy link
Contributor

@Sjerty Sjerty commented Feb 25, 2024

Summary

Added two game settings under "User Interface" category, toogle stamina bar and network traffic info and made prefabs for toogle, slider and dropbox, for easier adding of new items;

PR checklist

  • The game builds properly without errors.
  • No unrelated changes are present.
  • No "trash" files are committed.

Pictures/Videos

Made some changes, so the videos are not very accurate
Settings working:
https://youtu.be/feD-uWeeV9Y
Adding new categories:
https://youtu.be/aBjCcgqHVzc

Testing

Host a game, and try changing settings under "User Interface" category.
Also you can use new prefabs to try and create new category, just drop down category prefab under "Content" and add some interactive element prefabs

Changes

Added Holder under "Game Settings" object, added a Scroll View, in it's content added new category prefab "User Interface", it's consist of "Header" object containing label , and "Settings" which contains different settings made of prefabs, two active toogles, and two inactive examples - slider and dropdown.

Unfortunatly prefabs are configured the way so they'll work as intendent only in this Game Settings window :C
And because of elasticity, you will be able to scroll even without scrollbar, I think this could be fixed if we create a wrapper for RectTransform, and handle toogling "Vertical" checkbox, when scrollBar is inactive.

Also didn't add saving of this game settings, as it wasn't stated in the Issue

Related issues/PRs

#1281

Closes #1281

@Sjerty
Copy link
Contributor Author

Sjerty commented Mar 5, 2024

Rebased changes onto develop


namespace SS3D.UI.Settings
{
public class UserInterfaceSettings : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is needed, we already have a ScriptableSettings to add the bandwidth display. I also don't agree at all with having stamina and bandwidth be in the same script in any form, they are not related and if we want to add more stuff it will quickly become a gigantic class.

I really don't see the need for this class at all


namespace SS3D.UI.Settings
{
public class UserInterfaceSettings : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

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

A user interface settings class would have more global settings, such as global scale, theme.

@joaoburatto
Copy link
Contributor

I'd invert the dependency, instead of a class having a reference for each UI that has to be disabled and create a method for each UI that needs to be disabled, I would make the UI that registers itself to a class that displays the toggles. Not hardcoding everything

@joaoburatto
Copy link
Contributor

For example, the BandwidthDisplay class registers itself in a GlobalUIVisiblityController, by calling something like GlobalUIVisiblityController.RegisterToggle() and by passing the BandwidthDisplay object.

@Sjerty
Copy link
Contributor Author

Sjerty commented Mar 5, 2024

Thank you for comments!
I'll try to redo the settings by your notes

@Sjerty
Copy link
Contributor Author

Sjerty commented Mar 15, 2024

Tried to make some changes, based on my understandings, hope I'm moving in a right direction!
Also suggestions on how to improve it are always appreciated C:

@stilnat stilnat self-requested a review March 24, 2024 18:03
@joaoburatto joaoburatto merged commit eab45ae into RE-SS3D:develop Apr 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In-game Settings
3 participants