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

Cloud Command System Migration #279

Conversation

Aurelien30000
Copy link
Member

@Aurelien30000 Aurelien30000 commented Aug 12, 2023

Introduction

This is the first pass of the cloud command migration for FAVS. There will be a second one to restore old-fashioned command syntax, tracked as #81.

I have to highlight that this is my first experience with this, must-say wonderful, command system. I have spent a lot of time reading every available documentation and code piece. If @Citymonstret wants to take a look and maybe give us tips to avoid "ugly" workarounds or handling, we would be glad!

The whole pull request has been tested, not yet thoroughly, you can have a global overview, but it is not really ready.

If you have any question or remark, do not hesitate!

General Command Management

I have opted to use the annotations extension of the cloud command system. In my opinion, this is better suited to the current brush format handling which is all done inside brush classes.

Executors have been kept and brush & performer command are still handled inside their classes.

  • Snipe class has been extended for a usage as a commander, because FAVS relies on a lot on this class.
  • CommandRegistry is the main place for the whole handling behind the scenes. Otherwise, commands are registered as usually done in cloud, with some specific annotations when needed.

SniperCommander class is the commander to use with cloud command system. If the player exists, it returns its sniper. Otherwise, it returns a simple SniperSender, similar to CommandSender.

Command Manager

FAVS uses the paper command manager, when available, to enjoy some improvements. Falls back to bukkit command manager otherwise.

  • Async tab-completions are enabled if available.
  • Snipe, PerformerSnipe & Toolkit classes are registered into the injector in order to be injected in command methods.
  • Command exceptions are adapted and customized with the FAVS message syntax.

Command Post-Processor

FAVS requires the command post-processor ins order handle specific FAVS behavior.

  • Handles the @RequireToolkit annotation, makes sure the toolkit is available and the value stored.
  • Handles the @DynamicRange annotation, used to define a range from non-constant variables, using reflection.
  • Prepares the brush & performer when needed, their Snipe and stores them.

Annotations & Parser

FAVS uses some annotations to facilitate development, based on common rules and behaviors.

  • Handles the @RequireToolkit annotation, modifies the command meta.
  • Handles the custom command execution method handler, which should differ for brush & performer. Cloud commands are designed to live in a class instance, this is not suitable to the current management of brush & performer. I have opted for a custom execution method which uses the brush & performer instance from the execution context instead of the base instance. This avoid extra parameters for each command method.

Arguments

FAVS needs a lot of custom arguments for either factories, registries, custom types, custom needs, etc.

Suggestions & parsers are also declared via annotations in custom classes.

Other Changes


  • All classes related to internal command managing from VS have been removed.
  • FastAsyncVoxelSniper class has been removed. As far as I know, this class was useless and is now for sure.
  • Some classes and methods have been added or refactored, but the overall code base is the same to try keeping maximum compatibility.
  • Some translations have been reorganized or removed.
  • Some code format has been modified, there will be another pull request next year hopefully to unify comments format.
  • Improvements to brush properties loading. Previously, all aliases were loaded, subsequently loading the same brush several times.
  • Modern switch syntax has replaced old ones.
  • General improvements.

Known Problems:

Submitter Checklist

@Aurelien30000 Aurelien30000 requested a review from a team as a code owner August 12, 2023 17:06
@Aurelien30000 Aurelien30000 marked this pull request as draft August 12, 2023 17:06
@github-actions github-actions bot added the Feature This PR adds a new feature label Aug 12, 2023
@Aurelien30000 Aurelien30000 added Enhancement This PR enhances an existing feature Major feature labels Aug 12, 2023
@Aurelien30000 Aurelien30000 marked this pull request as ready for review August 12, 2023 17:10
@NotMyFault
Copy link
Member

I don't have the time or knowledge about cloud to review this PR properly. Maybe @Citymonstret could take a look or members from the community could test the builds.

@OneLiteFeather
Copy link
Member

Maybe @TheMeinerLP could do that if he has some time in the next days, somehow I can't ping him, but he uses CLOUD quite often in our plugins.

@Aurelien30000
Copy link
Member Author

I'd be grateful if you could test the whole! The code base diff is huge but it's mainly due to moving code blocks from the legacy handleCommand method to their dedicated command methods. The rest is about command handling and should be reviewed as usual.

@OneLiteFeather
Copy link
Member

I'd be grateful if you could test the whole! The code base diff is huge but it's mainly due to moving code blocks from the legacy handleCommand method to their dedicated command methods. The rest is about command handling and should be reviewed as usual.

I am going to do that today in game hopefully!

@OneLiteFeather
Copy link
Member

#286 all the found bugs related to this (some might be added later if I find any)
I know it is not best practise to collect them this way, but it might be easier to look out for them.

Copy link
Member

@OneLiteFeather OneLiteFeather left a comment

Choose a reason for hiding this comment

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

Looked out for in game functionality of brushes and their new cloud commands and some small errors / spelling I can identify, but I can't know if the code is "right" 100% for this PR (I am not a well trained developer). Opened some issues regarding the functionality, the implementation of CLOUD looks "good" (no errors I found, just the ones that are in this review) from user view.

Copy link
Member

@OneLiteFeather OneLiteFeather left a comment

Choose a reason for hiding this comment

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

My review is done, let's go :)

@TheMeinerLP
Copy link
Member

I started also to review it

@OneLiteFeather
Copy link
Member

Hey @TheMeinerLP this is a reminder that you wanted to review this. You might forgot to continue :)

@NotMyFault
Copy link
Member

Do you want to make a (public) beta of this PR to allow people to test it in the field? I think that would make more sense than keeping it as PR until us maintainers spot all (code) related issues.

Also, conflicts.

# Introduction

**This is the first pass of the cloud command migration for FAVS.**
There will be a second one to restore old-fashioned command syntax, tracked as IntellectualSites#81.

_I have to highlight that this is my first experience with this, must-say wonderful, command system. I have spent a lot of time reading every available documentation and code piece. If @Citymonstret want to take a look and maybe give us tips to avoid "ugly" workarounds or handling, we would be glad!_

The whole pull request has been tested, not yet thoroughly, you can have a global overview, but it is not really ready.

If you have any question or remark, do not hesitate!

--

# General Command Management
I have opted to use the annotations extension of the cloud command system. In my opinion, this is better suited to the current brush format handling which is all done inside brush classes.

**Executors have been kept and brush & performer command are still handled inside their classes.**

- ``Snipe`` class has been extended for a usage as a commander, because FAVS relies on a lot on this class.
- ``CommandRegistry`` is the main place for the whole handling behind the scenes. Otherwise, commands are registered as usually done in cloud, with some specific annotations when needed.

**``SniperCommander`` class is the commander to use with cloud command system. If the player exists, it returns its sniper. Otherwise, it returns a simple ``SniperSender``, similar to ``CommandSender``.**

# Command Manager
FAVS uses the paper command manager, when available, to enjoy some improvements. Falls back to bukkit command manager otherwise.

- Async tab-completions are enabled if available.
- ``Snipe``, ``PerformerSnipe`` & ``Toolkit`` classes are registered into the injector in order to be injected in command methods.
- Command exceptions are adapted and customized with the FAVS message syntax.

# Command Post-Processor
FAVS requires the command post-processor ins order handle specific FAVS behavior.

- Handles the ``@RequireToolkit`` annotation, makes sure the toolkit is available and the value stored.
- Handles the ``@DynamicRange`` annotation, used to define a range from non-constant variables, using reflection.
- Prepares the brush & performer when needed, their ``Snipe`` and stores them.

# Annotations & Parser
FAVS uses some annotations to facilitate development, based on common rules and behaviors.

- Handles the ``@RequireToolkit`` annotation, modifies the command meta.
- Handles the custom command execution method handler, which should differ for brush & performer. Cloud commands are designed to live in a class instance, this is not suitable to the current management of brush & performer. I have opted for a custom execution method which uses the brush & performer instance from the execution context instead of the base instance. _This avoid extra parameters for each command method._

# Arguments
FAVS needs a lot of custom arguments for either factories, registries, custom types, custom needs, etc.

**Suggestions & parsers are also declared via annotations in custom classes.**

# Other Changes
- All classes related to internal command managing from VS have been removed.
- ``FastAsyncVoxelSniper`` class has been removed. As far as I know, this class was useless and is now for sure.
- Some classes and methods have been added or refactored, but the overall codebase is the same to try keeping maximum compatibility.
- Some translations have been reorganized or removed.
- Some code format has been modified, there will be another pull request next year hopefully to unify comments format.
- Improvements to brush properties loading. Previously, all aliases were loaded, subsequently loading the same brush several times.
- Modern switch syntax has replaced old ones.
- General improvements.

# Known Problems:
- There is currently one small issue with static/literal arguments and their aliases. Tab-completions are not handled for all aliases due to https://github.com/Incendo/cloud/blob/master/cloud-core/src/main/java/cloud/commandframework/arguments/StaticArgument.java#L134.
- Brigadier extension is voluntarily not used due to some incompatibilities with FAVS commands syntax.
- Fix ``/favs debugpaste`` link (format & clicking).
- Move commands reloading logic via ``/favs reload`` (broken & no longer needed).
- Re-add ``meteor`` as an alias of the comet brush.
- Set max brush size to a new reasonable value (500).
- Inform the sniper about any exceptions (unexpected errors).
@Aurelien30000
Copy link
Member Author

Sorry, conflicts fixed. A public beta would be nice, the whole PR is globally ready!

@NotMyFault NotMyFault changed the base branch from main to cloud-beta October 15, 2023 13:46
@NotMyFault NotMyFault merged commit efa25e4 into IntellectualSites:cloud-beta Oct 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This PR enhances an existing feature Feature This PR adds a new feature Major feature
Projects
None yet
4 participants