Better hud#2377
Conversation
note: if this were to be reverted, parents weren't set.
# Conflicts: # src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/PlayerListWidget.java
actually have no idea why this line got added
# Conflicts: # src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/EssenceWidget.java
kevinthegreat1
left a comment
There was a problem hiding this comment.
I would suggest a lot more comments. For example, things like TabHudWidget is widgets from the tab, not related to whether it is being shown on the fancy tab layer or fancy hud layer. Also, document how the new config works, etc.
In addition to code review:
- The open widget config screen button in skyblocker config doesn't work.
- Removing a widget with the delete key doesn't seem to work
- Missing a few textures
- The "Auto Screen Anchor" tooltip is quite useless, maybe explain what it does in the tooltip?
- None of the tab hud widgets seem to be showing up on any island? See attached screenshot, I'm expecting a lot of default tab widgets there, but there are none. (This is my first time running the pr.) If this is intended, due to me editing a different island than the one I'm on (which is unknown) then there should be at least some placeholders based on what widgets are expected there, or what widgets are configured. Or perhaps the widgets config really should be connected with the hypixel
/widgets?
| .name(Component.translatable("skyblocker.config.uiAndVisuals.tabHud.fancyWidgetsList")) | ||
| .description(Component.translatable("skyblocker.config.uiAndVisuals.tabHud.fancyWidgetsList.@Tooltip")) |
There was a problem hiding this comment.
I don't think this text is correct?
There was a problem hiding this comment.
it is, in fact, incorrect. Copy paste oopsie
| .binding(defaults.uiAndVisuals.tabHud.enableFancyTab, | ||
| () -> config.uiAndVisuals.tabHud.enableFancyTab, | ||
| newValue -> config.uiAndVisuals.tabHud.enableFancyTab = newValue) |
There was a problem hiding this comment.
If this does the same as tabHudEnabled, I think we should keep it as tabHudEnabled.
There was a problem hiding this comment.
It does not, tabHudEnabled removed also the tab widgets that were moved on the HUD. This just enables/disables the fancy tab when you press tab you know? Now that I think about it showVanillaTabByDefault can already do enableFancyTab's job, so one could be removed
| .option(ButtonOption.createBuilder() | ||
| .name(Component.translatable("skyblocker.config.farming.farmingHud.config")) | ||
| .prompt(Component.translatable("text.skyblocker.open")) | ||
| .action(screen -> Minecraft.getInstance().setScreen(new WidgetsConfigurationScreen(Location.GARDEN, FarmingHudWidget.getInstance().getInternalID(), screen))) | ||
| .build()) |
There was a problem hiding this comment.
I think you should add back these buttons across all the categories.
There was a problem hiding this comment.
I plan on removing the "enable x hud" toggles across all configs as they literally do nothing right now ✨
And I also plan to move the widget configs from there to the per widget thing in the new system to have them in the side panel (stuff like farming hud and end hud options)
didn't get around to doing it to hopefully avoid merge conflicts a smidge, in case those configs get touched in the time this gets reviewed. Removed these buttons so it would compile
| import de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder; | ||
| import de.hysky.skyblocker.skyblock.tabhud.screenbuilder.WidgetManager; | ||
|
|
||
| public class ConfigScreenBuilder extends ScreenBuilder { |
There was a problem hiding this comment.
You can make ScreenBuilder generic.
There was a problem hiding this comment.
Not sure what you mean
There was a problem hiding this comment.
I just mean add <T exntends Layer> to ScreenBuilder and ConfigScreenBuilder extends ScreenBuilder<ConfigLayer>, which gets rid of some casts.
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| // I would like to remove the tab logic from the hud layer by making it a subclass but that's already taken by ConfigLayerBuilder |
There was a problem hiding this comment.
I'm assuming ConfigLayerBuilder needs both hud and tab LayerBuilders. In that case ConfigLayerBuilder should probably contain a LayerBuilder instead of extending LayerBuilder.
There was a problem hiding this comment.
it has been a month now and exams passed by so I forgot a thing a or two as to why I didn't do that. But I did think of doing that, the issue that I had is that I would have to make some things public/have getters and I didn't really want to, and maybe another issue I forgot
There was a problem hiding this comment.
Well you cant extend two things, unless you can make one of them into an interface.
|
I do think this is a lot more intuitive though, so thanks! |
Well first fancy tab needs to be enabled duh. Making it connected to the hypixel
Advantages:
So uuuhh maybe I guess 🤷 Other issues in the "main review message" should be easy to fix and are due to oversights and voodoo magic (the description of the auto screen anchor button IS there, just... with the wrong translation key somehow, and i was 99% sure i committed the textures lol) |
|
I think separating them works, just have a difficult to miss note on the screen when configuring with |
|
also, I think you should distinguish hypixel widgets in both the list (when adding widgets) and the preview. Like add a [Hypixel] in front of the name or something, both in the list and the preview. |
The question is where
Again, where. On the widget itself or in the side panel? |
# Conflicts: # src/main/java/de/hysky/skyblocker/skyblock/ItemPickupWidget.java # src/main/java/de/hysky/skyblocker/skyblock/garden/FarmingHudWidget.java # src/main/java/de/hysky/skyblocker/skyblock/tabhud/config/WidgetsConfigurationScreen.java
|
For the notice, just put it below the current instructions in the middle of the screen. For the per widget, I’d say everywhere where the name appears, so in the title of the widget itself (only in preview), in the widget list when adding it, and maybe on the right panel as well. Just add like [Hypixel] in front of the widget name/title. |
|
And for the first part, the general note, I would also say whether the current page is skyblocker hud or /widgets. |
the config screen doesn't replace /widgets anymore |
Should probably have said that earlier but I'm not sure if it's a good idea. i'm guessing you're suggesting that because you think that it will be confusing if the widget disappears cuz TAB ran out of space. So saying that it's from hypixel might explain it? |
So the only way to accurately see the preview for all widgets is using skyblocker hud on the current island? I’m mostly worried about people trying to configure for other islands and either things not showing up in the preview or disappearing when actually going on the island. |
The contents of the widgets may be different than what they expect, but the widgets configured in the other location will be in the correct spot and displayed correctly. Fancy tab stuff will indeed be different as previously discussed. |
I’m talking about hypixel widgets, aka fancy tab. Right now it would be very confusing for a user configuring another island’s hypixel/fancy tab widgets and seeing nothing or different widgets than what’s actually should be there. |
|
Yea. Well like I said earlier there's a 2 solutions to that:
If you have other ideas then shoot away |
I swear I finished it a bit back but apparently not
hey it's that thing again.
This PR hopefully makes the hud system less confusing. hopefully.
not everything is done to hopefully ease merge conflicts and would be done at a later date.
misssing:
instanceofchecks for config stuffA quick run through of changes I guess:
ScreenBuilderinstances, only one whose config is changed/widgetsis decoupled from the config screen and has its own option for the fancy stuffI would appreciate some thoughts on the comment at the top of LayerBuilder