-
Notifications
You must be signed in to change notification settings - Fork 60
Implement a better options menu #42
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
base: master
Are you sure you want to change the base?
Conversation
Bixilon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the initial code review, have not looked deeper inside nor tested it.
...in/java/de/bixilon/minosoft/gui/rendering/gui/elements/input/button/AbstractButtonElement.kt
Outdated
Show resolved
Hide resolved
...in/java/de/bixilon/minosoft/gui/rendering/gui/elements/input/button/AbstractButtonElement.kt
Outdated
Show resolved
Hide resolved
...in/java/de/bixilon/minosoft/gui/rendering/gui/elements/input/button/AbstractButtonElement.kt
Outdated
Show resolved
Hide resolved
|
|
||
| private companion object { | ||
| val CLICK_SOUND = minecraft("ui.button.click") | ||
| val CLICK_SOUND = "minecraft:ui.button.click".toResourceLocation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val CLICK_SOUND = minecraft("ui.button.click")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe theres some issues with sounds, I couldnt get them to work, no ideas why...
src/main/java/de/bixilon/minosoft/gui/rendering/gui/elements/input/slider/SliderElement.kt
Outdated
Show resolved
Hide resolved
...main/java/de/bixilon/minosoft/gui/rendering/gui/gui/screen/menu/options/CloudSettingsMenu.kt
Outdated
Show resolved
Hide resolved
...main/java/de/bixilon/minosoft/gui/rendering/gui/gui/screen/menu/options/CloudSettingsMenu.kt
Outdated
Show resolved
Hide resolved
| updateDisabledStates() | ||
| } | ||
|
|
||
| private fun updateDisabledStates() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The profile provides delegates. You can easily observe it. Justs create a profile synced button (or so), that observes the profile and updates the button when that "event" is triggered. No need for polling.
src/main/java/de/bixilon/minosoft/gui/rendering/gui/gui/screen/menu/options/OptionsMenu.kt
Outdated
Show resolved
Hide resolved
src/main/java/de/bixilon/minosoft/gui/rendering/sky/clouds/CloudRenderer.kt
Outdated
Show resolved
Hide resolved
|
Slider bars still are wonky, I will rewrite that. |
|
well that rebase fucking died on me. |
…fullbright. Theres currently many issues but I taped it together and it kinda works(?) it currently has these problems: * Doesnt include all options * Cloud options when changed override "moving clouds" setting. * Sliders sometimes dont set to exact numbers D: * Slider system isnt exactly same as minecrafts, which I am not sure how to make work... It doesnt feel as smooth as minecrafts sliders... * The Chat width and height text boxes cant be changed, the boxes needs a special input block similar to... you guessed it! * The tooltips dont follow the cursor, I couldnt find out a way to implement it that isnt a taped together ugly tech. * Sliders are missing the grayed out textures, currently only making them slightly transparent, at least til I can bother to fix the "knob" texture being an actual knob :') * Settings tabs should probably be reorganized in a better manner, current organization feels a little all over the place. * Translation strings needs to be added, I gave up at some point :') * I dont have headphones with me... Does the master volume slider work?? * Graying out of buttons doesnt work after rebasing. * Hi, please dont be mean :) Update SliderElement.kt fixed the updated version of minosoft (graying out buttons is broken)
fixes to make it actually compile, damn it.
Slider bars still are wonky, I will rewrite that. Controls are still not working, I am unsure how can I make something that looks good with current UI and menus. Do we... even support resource packs lmao?
…t and mouse capturing for it.
my bad for using outdated file haha :')
…n can be fullfilled This excludes cases if a combination for "E+T+Y" exists and you press "E" to open the inventory, you can press Y+T+E on those cases so neither chat or inventory opens and you can use the combination for that... I think even vanilla minecraft bothered with their controls menu this much I swear...
Bixilon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So (finally making a review):
All the possible languages are in code, that feels wrong. Can be move that to a json file in the assets? 371dc9a
I don't really care about the old UI system anymore, so I am kind of blindly accepting it.
From UI perspective:
- feels weird that you can smoothly slide the biome radius like it was a decimal value.
- scrolling in controls makes me change the hotbar slot
From code:
- there is a lot of duplicated code or unused variables
- please remove unnecessary comments
This style is not good:
private val textFilteringButton: ButtonElement
...
textFilteringButton = ButtonElement(guiRenderer, formatEnabled("menu.options.chat.text_filtering", chatProfile.textFiltering)) {
chatProfile.textFiltering = !chatProfile.textFiltering
textFilteringButton.textElement.text = formatEnabled("menu.options.chat.text_filtering", chatProfile.textFiltering)
}
this += textFilteringButton
This whole part can be squashed into
this += BooleanDelegateButton(guiRenderer, "menu.options.chat.text_filtering".i18n(), delegate = chatProfile::textFiltering)
(and it handles translation, getting, setting, observing and formatting)
I added those changes, see 0e7c250
Translation are messed:
protected fun translate(key: String): String {
return IntegratedLanguage.LANGUAGE.forceTranslate(key.i18n().translationKey).message
}
The text formatting does that automatically, no need for anything. just use the translation key as text (in a base component).
I don't really like that the in game ui is responsible for most settings, why not in eros?
This is a really big first PR, it might make sense to split this.
Is this AI coded? I can not accept pure AI contributions.
| private val SCROLLBAR_TRACK_COLOR = RGBAColor(40, 40, 40, 180) | ||
| private val SCROLLBAR_THUMB_COLOR = RGBAColor(120, 120, 120, 220) | ||
|
|
||
| val CATEGORIZED_KEYBINDINGS: Map<String, Map<ResourceLocation, KeyBinding>> = linkedMapOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are all keybindings duplicated here?
| import de.bixilon.minosoft.gui.rendering.gui.input.mouse.MouseButtons | ||
|
|
||
| /** | ||
| * Interface for GUI elements that can capture mouse input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't want such comments, everything should be self explaining .
| * This file exists purely for sliders in menus to be able to capture mouse outside their bounds. | ||
| * Could have more uses in future... | ||
| */ | ||
| interface MouseCapturing { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why just relativeX? Why not relativeY? Why relative at all and not absolute?
| } | ||
|
|
||
| fun onKey(code: KeyCodes, pressed: Boolean, handler: InputHandler?, time: ValueTimeMark) { | ||
| // Find all satisfied bindings that use this key and track their key counts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if thats good. The current behavior is depending on the order of registration. Shouldn't all of them just be triggered? But that breaks (e.g. Q and CTRL + Q). The whole keysystem feels somehow not great anymore.
This function needs refactoring, feels really long and clumsy now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with the current system was if you assign Shift+Q as a key for walking forward character drops item in inventory while achieving shift+q key as well, and if the combination keys are used for movement, and stopping pressing the keys isnt detected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe controlling in general needs a rewrite before properly supporting combination keystrokes...
| * Creates a unique signature for a key binding based on its key codes. | ||
| * Used for duplicate detection. | ||
| */ | ||
| fun getBindingSignature(binding: KeyBinding): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a string? Why not just the set (you can use EnumSets from kutil)
Currently doesnt have a menu for resource packs.
Theres an issue with updating the clouds when you change settings, I dont think its an issue on options menu side.
Its missing some options that would be nice to have, open for suggestions on that.
Chat settings should have a proper textbox implementation for setting width...