Skip to content
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

MekaTool Radial Profiles #7471

Open
wants to merge 1 commit into
base: 1.18.x
Choose a base branch
from

Conversation

mewacaser
Copy link
Contributor

Changes proposed in this pull request:

So I got this idea from Mekanism-Feature-Requests#282

It is mostly working and it is awesome to use.
Currently limited to 6 profiles and while it has to be hardcoded to be in the enum there is no reason there couldn't be more, and potentially even disable them based on a numeric property

That being said there are issues with how I did it that probably need to be fixed.
The server sync kinda just piggybacks off of the module.save currently which works, but isn't ideal.
It's currently dealing with the NBT directly instead of using the config.set methods so that the module.save doesn't overwrite it, I have some potential solutions in mind, but I'm not sure which way would be best.
It only saves an option which you have changed while on that profile. Which if there was a way to clear a profile could actually be a feature.
The save button in the GUI doesn't do anything right now.
The profile names can be changed with the GUI, but it's static so it's the same for every MekaTool.

This also kinda makes the handle mode key option on the tool modules obsolete. So we might want to remove that on them.

@Tyrannicodin
Copy link
Contributor

Tyrannicodin commented May 6, 2022

6 seems a good number to me, and we could maybe use the handle mode key to open the menu instead of switching modes if it'll just be obsolete.
As for nbt storage, we might want to store it globally for all meka-tools, because if you're using on meka-tool and switch to another (no clue why you'd do that), you would still want to be able to use your configs.

@mewacaser
Copy link
Contributor Author

To clarify the handle mode key option previously decided if trying to change the mode would change the mode on that module, the key itself is still being used for the radial menu, but it can change the mode for any config. What I was suggesting possibly removing was just the boolean on the module to say whether it handled the key.

The NBT profiles are currently on the itemstacks, which is also where the module data is, if you switched to a different mekatool it could have different modules/configs/profiles. I don't think we'd want that to be global, even though the profile names are currently.

@Tyrannicodin
Copy link
Contributor

I see what you mean now, and I definitely agree. You're probably right about radial wheels being NBT not global as well

@SirEndii
Copy link
Contributor

SirEndii commented May 7, 2022

It is mostly working and it is awesome to use.

Show me <:

@mewacaser
Copy link
Contributor Author

minecraft-1182-singleplayer-2022-05-07-13-40-25-xisvz1or-ygwc47bp_Ax91Gc9x.mp4

@mewacaser mewacaser marked this pull request as ready for review May 7, 2022 23:16
@mewacaser mewacaser changed the title MekaTool radial profiles kinda working, more GUI work needed MekaTool Radial Profiles May 10, 2022
@mewacaser
Copy link
Contributor Author

mewacaser commented May 10, 2022

I think everything is resolved on this except for the static mode names. I don't see a way to fix that so I think it's done unless someone else can think of something.
Edit: Forgot about the commented lines I left in there in case syncing the state to the current mode was something we wanted.
Currently you can change the configs however you want and not corrupt the current profiles, you would have to press the save button for that specific profile and it will save the current options to it. The question is whether we want to have changing the options in module tweaker save to the current profile or not? And whether it should be initiated by the client (more traffic) or the server (less configurable)? If it's on the client it could actually be a config option whether or not to sync, which might be nicest for the users.

@mewacaser mewacaser force-pushed the feature/mekatool_radial_profiles branch 3 times, most recently from c03dc4c to f6edc72 Compare May 12, 2022 05:49
@mewacaser mewacaser force-pushed the feature/mekatool_radial_profiles branch from f6edc72 to 567ca32 Compare May 27, 2022 16:34
Rebased, cleaned up, and squashed.
Squashed commits:
MekaTool radial profiles kinda working, more GUI work needed
Added packet to save mekatool profile, save client config, gui fixes
Small changes to the enum
Added profile hud string and removed handle mode key from efficiency module
@mewacaser mewacaser force-pushed the feature/mekatool_radial_profiles branch from 567ca32 to 6525af2 Compare June 7, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants