Conversation
|
Hey @litetex, sorry I didn't see this a moment sooner! Another PR for 1.21.11 has just been merged in. If you have any improvements though, feel free to update this PR or make a new one! Thanks heaps :) |
Ported from previous version as there were changes done in the meantime. See BVengo#33 * Improve project setup/templating * Fix and improve rendering of options screen * Now responsive * Requires less rendering work * Rendered correctly on 1.21.11 * Fix memory leak due to `currentSoundInstance` * Fix sounds keeping playing after leaving the screen * Correctly mixin to `SoundOptionsScreen` * Improve code formatting in some places * Improve file saving * Removed old configuration migrations as they are probably outdated by now
c8557bc to
124bcff
Compare
|
@BVengo Updated as requested. I also fixed 2 additional problems:
|
BVengo
left a comment
There was a problem hiding this comment.
Thanks so much litetex, it looks great.
I've added a few comments (mostly clarifications for my own understanding, but some changes). Could you please check them out, and then I'll see if we can get this merged in 👍
There was a problem hiding this comment.
Separation by files made it easier to quickly parse, so why inline the SoundReloadListener?
There was a problem hiding this comment.
Well the external file was quite big, but yeah when I now think about it, it might have been the better way.
I'm however not sure why this is even needed in the first place...
The changelog of 1.2.4 states:
This is useful not just for when you add new resource packs, but also when you modify your config file directly.
- AFAIK resource packs don't add new sounds
- Why slow down the loading of resource packs? Why don't just add a button in the UI that says "Reload config from disk" or something like this? I'm also not sure why someone would manually edit the config file in the first place while the game is running...
There was a problem hiding this comment.
-
They can add new sounds (datapacks too), used heavily in customised servers such as Hypixel, MCC, etc. I've had a lot of reports of them not properly loading, and this change fixed all issues by ensuring that sounds are re-checked during the refresh (and new ones added).
-
All the users who overload the value, but also useful for debugging. This wasn't the primary purpose, just a happy accident.
There was a problem hiding this comment.
-
They can add new sounds (datapacks too), used heavily in customised servers such as Hypixel, MCC, etc. I've had a lot of reports of them not properly loading, and this change fixed all issues by ensuring that sounds are re-checked during the refresh (and new ones added).
-
All the users who overload the value, but also useful for debugging. This wasn't the primary purpose, just a happy accident.
| this.resetButton = new TriggerButtonWidget("reset", 0, 0, BUTTON_SIZE, BUTTON_SIZE, | ||
| (button) -> { | ||
| this.volumeOption.set(this.calcSliderValue(VolumeData.DEFAULT_VOLUME)); | ||
| ((ResettableOptionWidget) this.volumeSlider).resetValue(); | ||
| this.updateElementWidths(); | ||
| this.updateElementPositions(); | ||
| }); |
There was a problem hiding this comment.
Why does this need to resize / reposition elements when reset? It should just be resetting the volume data which shouldn't have any positional impact (as far as I can tell).
There was a problem hiding this comment.
Good catch, this is no longer needed as the Slider is no longer re-created when resetting.
| @Override | ||
| protected void addFooter() { | ||
| LinearLayout directionalLayoutWidget = this.layout.addToFooter(LinearLayout.horizontal().spacing(8)); | ||
|
|
||
| // To individual volume options screen | ||
| AllSoundOptionsScreen volumeOptionsScreen = new AllSoundOptionsScreen(this, this.options); | ||
| this.addLayoutButton(this.minecraft, directionalLayoutWidget, Translations.SOUND_SCREEN_TITLE, volumeOptionsScreen); | ||
| this.addLayoutButton(this.minecraft, directionalLayoutWidget, CommonComponents.GUI_DONE, this.lastScreen); | ||
| } |
There was a problem hiding this comment.
Should this remain a WrapOperation for compatibility purposes?
See f9f0d37
There was a problem hiding this comment.
It's not possible to do a WrapOperation here as the method must be overriden only for the SoundOptionsScreen.
AFAIK there is no way to override a method with a WrapOperation.
The previous solution was not efficient as it required a check for SoundOptionsScreen in every OptionsScreen
Also:
- I'm not aware of any other mod that changes this menu so conflicts are not expected in the first place
- If another mod changes this footer the mod will likely also do changes to the sounds and this should make it completely incompatible in the first place
- If another mod also adds components to the footer there will likely be not enough space in the first place to display both of them. If you really want good compatibility: Simply add the "Individual sounds" button to the normal options in the list above as the footer is very limited in size. However I don't think that this is currently necessary.
There was a problem hiding this comment.
Fair call. I'd like to one day stick it in the margins (e.g. Distant Horizons, etc), but that's for a later date.
|
@BVengo |
Thanks heaps. I dropped a few replies, but will cover anything I missed when I'm next free :) |
[SQUASHED] Ported from previous version as there were changes done in the meantime. See BVengo#33 * Improve project setup/templating * Fix and improve rendering of options screen * Now responsive * Requires less rendering work * Rendered correctly on 1.21.11 * Fix memory leak due to `currentSoundInstance` * Fix sounds keeping playing after leaving the screen * Correctly mixin to `SoundOptionsScreen` * Improve code formatting in some places * Improve file saving * Removed old configuration migrations as they are probably outdated by now Restore sound reload listener Improve parsing of config - Readd old logic Apparently a ton of people are still using 1.20.1 -.- This is no longer required as the slider is no longer re-created when clicking reset As requested: Only do patch version bump Cleanup, call onClose from upstream
b35a9f6 to
8c3cdc4
Compare
Contains various fixes and improvements and updates the mod for 1.21.11:
currentSoundInstancebeing not cleared after leaving the options screenSoundOptionsScreen