FTBTeams: Commands refactor, pre-PR RFC

[Starkrights]

Hello!

Background

I've been using packs with the ftb suite of mods for some time now. The functionality of some of the commands are a bit wonky when it comes to configuring team settings. In specific, when trying to run a command like ftbteams party settings_for <team> ..., in some cases, the command returns a typical user-feedback message, and in other cases[1], it causes an internal exception & chokes out a 400ms tick.

What I'd like to do

I'd like to submit a PR that just fixes the bugs, but the code between FTBTeamsCommands.java and & its related files has some quirks. There's some interesting coupling going on[2] that confuses control/data-flow between classes, so I'd really like to make a refactor pass over it. Furthermore, I'd like to pose some suggestions for changes in the functionality that would probably benefit from the refactor being in place first.

• For example, currently it doesn't seem possible to edit player-party permissions through the command interface[3]. Maybe there's a reason that's been excluded, but I think it's reasonable for a server admin to want to be able to modify that in the case said player is offline (without restarting the server to manually edit their snbt file).
• I haven't been through the codebase deep enough to tell if it's possible,, but I think the settings management interface could be condensed into a singular command path as well.

TL;DR, I'd like to take a refactor pass over the FTBTeams command code to make it more concise, and extend/condense parts of its functionality.

Concession

I recognize that I'm poking a bear here- that is, the mod works for the majority of what it needs to, and obviously hasn't caused too many problems since the inception of the code I'm referencing (~99% 2-4 years old). So, if consensus was "yeah, that sounds like a looot of potential new instability", I'd totally understand.

Otherwise, please throw me input! If this sounds like something that could potentially be merged, I'd love to get started on it.

References

[x] references.

[0]: Attempt to execute `ftbteams party settings_for (...)`: Tick inflation does not occur on a normal/successful execution.

[1]: One I've confirmed so far is using a server-party name as an argument to ftbteams party settings_for. I recognize the ftbteams server settings command is intended for server-party settings as of now, but throwing an unhandled exception & causing a couple of 300ms ticks seems improper.

[2]: E.g. Why are we creating static methods in (e.g.) FTBTeamsArgument.java to extract data from a Command context, when those methods are only used in FTBTeamsCommands.java. It seems like this should really just be done in the FTBTeamsCommands.java class.

[MichaelHillcox]

@desht

[desht]

I'm not at all opposed to a more comprehensive review of how the FTB Teams command system is set up. It's old and janky, and does need a bit of attention. The reason it's never had an overhaul is (as you pointed out) it works in the majority of cases, and there's only so many hours in the day 🤣

So yeah, totally open to a refactor pass. If you have any questions about internals, feel free to ask...

[Starkrights]

That's great to hear!

Is there an ideal version/branch for me to fork from to start work on? I see we've got a main & dev branch, as well as <version>/main and <version>/dev branches per mcversion. My guess would be 1.21.5/dev or 1.21.1/dev?

Also out of curiosity, what's the branching workflow here? Or at least, what's the trigger for merging a version branch into the root main/dev?

[desht]

Start with the 1.21.1/dev branch, since that's still our main focus for public releases.

1.21.5/dev is basically dead - we have no plans to ever to a release on that version.

I do cherry-pick stuff out of 1.21.1/dev into our current dev branch (which tracks 1.21.8 right now), but I can handle the cherry-picking if you target 1.21.1/dev.

[desht]

Also out of curiosity, what's the branching workflow here? Or at least, what's the trigger for merging a version branch into the root main/dev?

The dev branch tracks code for the newest version of Minecraft we've ported to so far, so 1.21.8 in the case of FTB Teams. Although there's no public release for that version, and we have no short-term plans for one.

Older branches are named {VER}/dev and {VER}/main, e.g. 1.21.1/dev. We commit work to {VER}/dev on a regular basis, and when ready for a public Curseforge release, merge {VER}/dev into {VER}/main via a PR/review process. Currently VER is 1.21.1 for stuff that's actually released, and the latest MC dev branch (1.21.8 right now) for stuff we're in the process of porting up but don't intend to release in the near future.

[Starkrights]

My vision for command re-organization is more/less as follows:

• /ftbteams party (...)
  • Encompass all commands intended for regular players
    • eg create, join/leave, settings config, management
  • For management commands, all of them will apply to your current party, and respect party-level permissions.
    • i.e., regardless of whether or not you're an op, you're going to get bounced if you're not a member of a party, with the correct permissions.
• /ftbteams admin party (...)
  • A prefix for party commands that bounces if you're not OP (or a permitted role from FTBRanks)
  • Allows settings management for all parties, including player-parties
  • Instead of a separate top-level node for server-parties, we provide a /ftbteams admin party create_server node. No need for an /ftbteams admin party create node, as creating a personal party should be done thru /ftbteams party create

This'll collapse the top level nodes to just /ftbteams party and /ftbteams admin, which makes things a bit more straightforward. Other current top-level nodes like info, list, msg, and redirect_chat would all reorg under /ftbteams party.

I think this is pretty succinct, but suggestions/different directions are welcome!

Command-interface semantics

I've got one semantics question for you. Currently, /ftbteams party create is configured such that if a player is already a member of a party-team, the create node isn't shown or available for use. There's some others like this as well. I'm 50/50 on whether or not this is a good thing.

• Pro: the command-suggestion window is much less cluttered
• Con: the commands that are potentially available to a player are much more obscure.

We could either:

• allow users to execute all commands, and return descriptive error messages as to why they are/aren't allowed to execute them (eg "You do not have permissions for this command!" or "You are already in a party")
• OR, do the command restriction, and describe the constraints in the docs. I figure the docs will need an update anyways, so maybe this is the better route? Less visual-clutter, and players who want to know more are probably capable of searching for the docs page.

Thoughts?