Conversation
Bixilon
left a comment
There was a problem hiding this comment.
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.
val CLICK_SOUND = minecraft("ui.button.click")
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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...