Skip to content

Conversation

ReignOfFROZE
Copy link

@ReignOfFROZE ReignOfFROZE commented Mar 2, 2025

This PR adds saveable schematics for the Ring of Loki, adding keybinds to:

  • Save a schematic
  • Open the schematic management UI

The schematic management UI is the UI you will use to manage your schematics (as the name implies), allowing you to select the schematic you want to use, change the names of schematics, and delete existing schematics.

Other than that, it's a fairly straightforward change, and doesn't add any real functionality to loki, just some QoL that makes loki a bit easier to use.

Demo

Quick note: I am NOT attached to the icons or background that are included in this PR, and if anyone has alternatives they'd like to create/offer, I'm 100% on board with anything that you think may fit the aesthetic of Botania more, these were some I just pulled together in like 5 minutes.

It also should be noted this was my first time ever making a UI with any UI framework, not just MUI, so this UI code is probably absolutely terrible and if anyone smarter than I has comments, feel free to rip it apart.

Copy link
Member

@glowredman glowredman left a comment

Choose a reason for hiding this comment

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

I can't say much about how well ModularUI is used here, however I have a few points:

  • If you want to use ModularUI: make this feature optional. At the moment, ModularUI is introduced as a hard-dep (and not even declared as one). Botania is listed as active support, so introducing new deps must have a good reason. A cleaner solution (although quite some work) would be reimplementing the GUI without ModularUI.
  • The textures are to high-res. The files are way to big and the textures don't fit in with MC's style and are to detailed for the size they are displayed at.

@miozune
Copy link
Member

miozune commented Mar 2, 2025

Reimplementing GUI with vanilla is quite painful and kills the point in using library. Making it no-op when MUI is missing would be good.
And as I mentioned on Discord, please use MUI2 instead. I understand it's not fun to port things, but we need to move forward for deprecating MUI1.

@ReignOfFROZE ReignOfFROZE marked this pull request as draft March 2, 2025 18:51
@ReignOfFROZE ReignOfFROZE marked this pull request as ready for review March 2, 2025 22:59
@ReignOfFROZE
Copy link
Author

ReignOfFROZE commented Mar 2, 2025

The UI has been migrated to MUI2, the icons have been swapped for 16x16 icons, and I added method stripping and conditional checks to make sure that MUI is loaded for anything relating to the management UI.

@ReignOfFROZE ReignOfFROZE requested a review from glowredman March 5, 2025 15:59
@glowredman glowredman dismissed their stale review March 6, 2025 16:46

Changes requested were made, can't say much about the GUI code.

@glowredman glowredman requested review from miozune and a team March 6, 2025 16:46
@YannickMG YannickMG self-assigned this Apr 22, 2025
@YannickMG YannickMG self-requested a review April 22, 2025 00:09
Copy link

@YannickMG YannickMG left a comment

Choose a reason for hiding this comment

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

Thank you for your patience while MUI2 review bandwidth unlocked.

This is an awesome UI, but there's some work that must be done for it to reach the same level of polish found within the rest of Botania.

GUI Usability feedback

  • The elephant in the room is that deleting a schematic leaves an empty space in the row. The core issue is that the scroll widget doesn't truly delete it's children when this the delete button is clicked. edit: This also turns out to be problematic. A better solution appears to be dynamically rendering each Row and disabling the last row anytime a schematic is deleted.
  • The button that allows you to select a schematic uses a "checkbox" symbol which is slightly confusing. I suggest you invert the pattern so that all un-selected schematics show an empty checkbox and clicking that empty checkbox shows the existing symbol for the one which is currently selected.
  • It feels like it would be nice if you could return to the schematics screen after exiting out of the rename UI, if the rename UI even needs to be separate. (Maybe one could rename it directly from the list)
  • It is possible to rename a schematic to the exact same name as another schematic which looks like it overwrites them. This should ideally not be allowed, or result in 2 different schematics with the same name.
  • The UI remains open upon player death. While not critical, this can be fixed and it would be nice to do so.

Technical feedback

I see you have chosen to make this a clientside UI which makes use of custom made network packets for all serverside effects. This is nice if you don't want to learn too many ModularUI concepts at once but I believe it might be limiting the flexibility and features of your UI while complicating the synchronization model with those one-off packets.

What your GUI is doing is actually intrinsically tied to a state that needs to be stored on the server, given that all operations are writing to an ItemStack's NBT. The one grace you have is that there should be no action from the server which can actively modify your GUI except for the player dying so your synchronization doesn't really need to be 2-way. Still, 2-way synchronization could help you with the challenge of rendering the updated state of the itemstack after you rename or delete a schematic, as you would just be rendering the result of modifying the ring on the server.

All that to say you don't need to switch to a server-synched MUI2 UI, but you might want to consider it. GTNewHorizons/ModularUI2#23 has added an ItemStackGuiFactory which could make it quite easy to sync up the client and server to be reading the same Ring of Loki ItemStack and each instantiate/locale an IGuiHolder which would manage the communication. In your case it would be an extended version of GuiLokiSchematics. You could then use SyncHandlers to synchronize the list of schematics (GenericListSyncHandler) and other properties like the selected schematic.

edit: Forgot to mention, this repo would greatly benefit from having Spotless, Generic Injection and modern syntax enabled, but that shouldn't be in this PR.

Suggested course of action

I have pushed a commit to GTNewHorizons:Botania:feature/schematics-refactor with a suggested first step for regularizing the GUI code before further work is done. I wrote it mostly to validate that my feedback about avoiding static members and variables of type Object was sensible and doable. Though you do not need to do so, I suggest you just cherry-pick it onto your branch as a starting point as it addresses most of the feedback I've left.

Besides that, work with us in the Progressing on ModularUI2 to find the best way to get your UI to be fully functional! I know MUI2 has a feature which should handle closing the UI on player death for you but I can't find it right now and didn't want to delay the review any longer.

@YannickMG
Copy link

YannickMG commented Apr 23, 2025

I pushed an extra commit to GTNewHorizons:Botania:feature/schematics-refactor which handles visually adapting the schematics list when an item is removed. I don't mean to take over your PR, but I also didn't want to ask you to do something I didn't know a way of doing myself.

@Dream-Master Dream-Master requested a review from a team April 25, 2025 11:30
@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Apr 25, 2025
@combusterf
Copy link
Collaborator

My main concern with this is that this repository is meant to function like base Botania with better modding options. If we substantially want to deviate with that (i.e. turn the ring of Loki into a big multitool, and adding UI to a mod that has by design refused to do so) then it would have been better to add such functionality to BotanicHorizons

@ReignOfFROZE
Copy link
Author

ReignOfFROZE commented Apr 25, 2025

My main concern with this is that this repository is meant to function like base Botania with better modding options. If we substantially want to deviate with that (i.e. turn the ring of Loki into a big multitool, and adding UI to a mod that has by design refused to do so) then it would have been better to add such functionality to BotanicHorizons

I don't think this turns the ring of Loki into a big multi tool, it just adds some QoL to it. Additionally, the ring of Loki isn't from base Botania either, so I'd say we've already thrown base Botania's ideology out the window as it is.

Also, it wouldn't really be feasible to add all of this to BotanicHorizons unless you were to just add a ton of mixins to interact with the item class.

@YannickMG
Copy link

Interop is 100% possible without mixins if you add a proper API. That being said if this UI can be locked behind a config then there is no harm to those who want to play without it.

@ReignOfFROZE
Copy link
Author

Interop is 100% possible without mixins if you add a proper API. That being said if this UI can be locked behind a config then there is no harm to those who want to play without it.

It already is locked behind having MUI enabled on your instance (as well as having the keybinds bound), which I feel like is plenty of gating for experiences outside of NH as well as within NH

@YannickMG
Copy link

YannickMG commented Jun 2, 2025

I will note that by now MUI2 has been through a few breaking changes and the method collapseDisabledChild makes it much easier to avoid having holes in the list without resorting to the hacky workaround I proposed.

@YannickMG YannickMG dismissed their stale review July 1, 2025 14:00

My code was used, though it's not the best anymore since MUI2 has changed since.

@YannickMG
Copy link

image

On the latest version one of the keybind language keys is wrong.

Also I couldn't rename any schematics. It's quite possible the overcomplication which I used in my suggested code prior to the addition of collapseDisabledChild is part of the cause.

@Dream-Master Dream-Master requested a review from combusterf July 9, 2025 14:15
@Dream-Master
Copy link
Member

@combusterf are other changes are needed ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta
Projects
None yet
Development

Successfully merging this pull request may close these issues.