JorelAli / CommandAPI

A Bukkit/Spigot API for the command UI introduced in Minecraft 1.13
https://commandapi.jorel.dev
MIT License
532 stars 67 forks source link

[Enhancement] Adding more Argument Types #69

Closed Minenash closed 5 years ago

Minenash commented 5 years ago

(long time, no talk) Please describe your suggestion I was starting to use the api, when I realized it was missing some argument types, I tried to put these from most useful to least useful:

https://wiki.vg/Command_Data https://minecraft.gamepedia.com/Argument_types

Please supply examples of the desired inputs and outputs I mean these Arguments are explained above, I guess I'll do this one, as the wiki's description isn't great: /tell john Hey look at all the players: @a Shows to John "Hey look at all the players [Minenash, Steve, john]"


I know you can't add all 25 in at once. (I think at least), but The ones high up are definitely useful.

JorelAli commented 5 years ago

I'm aware that not all argument types are included in the command API - mainly because I didn't see a use for them in regular everyday Minecraft. Very few plugins these days would use something such as the rotation, or raw nbt where plugins would generally handle such information better using other events or querying the state of the world.

But yeah, there's no reason why I can't add them, I just felt that some were unnecessary given that most of them are used by the very technical commands (such as execute or scoreboard), which can be dealt with easily using plugins

Minenash commented 5 years ago

I agree some aren't too useful, and most can be done by the plugin author. Imo rotation is pretty useful, block_predict and block_state are very useful for stuff like /give, ranges are definitely nice to have. nbt to go with /give, modifing books, ect., color is also useful and less weird to use for stuff like settings, I put scoreboard stuff because you kinda already had a scoreboard thing, so I thought why not finish it, component for more advance funcatiality for users, time for timing stuff, swizzle is nice for block manipulation. The scoreholder is nice, because it allows devs to add custom players, so that the user MUST either use a real player/selector/* or a custom option by the devs.

The most useful and world definitely like to see added: rotation, column_pos, vec2 (realized you do have vec3, my bad), range, block state/predict, color, component, time, and nbt

Ones that do a job slightly better than current methods: Scoreholder, swizzle, item_slot, and long

Objective Stuff (not many would care about this, and I don't mind at all if you don't add theses): objective (I mean isn't this added but in a weird custom way, why not just use the actual one?), team (same inside as ^), and objective_criteria

And the ones I guess no one cares at all if they ever get added: resource_location, message, entity_anchor, entity_summon, nbt_tag, and nbt_path.

Edit:I thought entity_summon was like the entity type, then I realized that already existed, and after looking it up it make sense, yeah that one is (almost) useless

JorelAli commented 5 years ago

Fair enough. Once I've stabilised the API by dealing with the other issues, I'll work on adding the rest of the arguments

JorelAli commented 5 years ago

Okay, so with version 2.3 released and the CommandAPI officially "stable", let's discuss this in way more detail.

Clearing the confusion

We appear to be confused on what already exists and what doesn't. Here's what the CommandAPI already implements:

Minecraft argument name CommandAPI argument name
minecraft:color ChatColorArgument
minecraft:component ChatComponentArgument
brigadier:double DoubleArgument
minecraft:item_enchantment EnchantmentArgument
minecraft:entity EntitySelectorArgument
minecraft:entity_summon EntityTypeArgument
brigadier:float FloatArgument
minecraft:function FunctionArgument
brigadier:string GreedyStringArgument
StringArgument
TextArgument
brigadier:integer IntegerArgument
minecraft:item_stack ItemStackArgument
minecraft:block_pos
minecraft:vec3
LocationArgument
minecraft:particle ParticleArgument
minecraft:game_profile PlayerArgument
minecraft:mob_effect PotionEffectArgument

The suggestions, one by one

The thing about most of these suggested arguments is you have to think about it from both a Bukkit plugin developer's point of view, as well as a regular player's point of view. Most things can be handled easily by a plugin developer automatically (For example, getting a player's own location, rotation, inventory etc.). A player shouldn't really have to memorize complicated command execution arguments (hence, player arguments are amazing, ranged numbers or anything that's suggested is amazing).

Nonetheless, I'll try and voice my opinions on each of the ideas below:

Minenash commented 5 years ago

I agree with that some of these are neiche, and not "common".

rotation: I see what you mean, but if someone is trying to redo Minecraft commands into something that visually matches the server, then removing functionality just makes it annoying. Rotation would mostly be used for entity manipulation, and tp/warp. While most warp plugins will take the command runner's rotation, unless the guy used tp to make sure the rotation is exact, then the warp wouldn't be centered rotation wise.

column_pos and vec2 are the same as block_pos and vec3, but doesn't include the y value. These are useful for stuff like plots or anything else that doesn't care about y.

range: to be honest I don't know what's the difference between range and, int_range and double_range. But this range isn't "allows from x to y". The value itself is a range. 0..1, ..5, 1... an example of this in MC is /execute if score @s dummy matches 0..10 run say hi

item_slot: goes with the theme of not removing features from vanilla. Useful for staff clearing a specific item from a players inventory, or editing a kit without having a to load the kit to the player running it, him make the change, then save it.

I see how swizzle doesn't make that much sense, but block manipulation command are helpful, and the biggest block manipulation plugin not supporting 1.13 syntax doesn't negate that.

nbt and block: that makes sense.

color: I miss read the docs, I thought it meant stuff like &5

component: This isn't ChatCompnent. It's any json. This is useful for advance user features. While the json validation can be done by the plugin author, there's disadvantages:

Though I see where you're coming from, with the idea of an average player, however there are also staff, and some cool stuff for advance users.

JorelAli commented 5 years ago

Alright. There are a few more things I plan to discuss at a later date (tomorrow) about the complexity of implementing such features (Minecraft's swizzle is very complicated, and probably not implementable without some very complicated wrapper), but I'll leave all of that for tomorrow.

I look forward to implementing more arguments!

Minenash commented 5 years ago

Minecraft's swizzle is very complicated, and probably not implementable without some very complicated wrapper hmm, thought this would be the easiest. Look forward to the discussion and implementation!

edit: (relating to time)Again, a very niche argument which I don't see in wide use, but very useful indeed. Mainly useful for stuff like: tempbans, mutes, temp add/remove of permissions, anti-spam stuff, ect.

edit (again): I somehow missed the argument type, operation, which would go with the rest of the scoreboard arguments.

edit #3 So I was looking at the nms code to see how swizzle was implemented. And it seems to just return an EnumSet. So (in theory) there's three easy ways this could be done. By making a Swizzle class, and returning that; making a Swizzle Enum, converting the nms's enum to the api's; and returning an enum set of that, or return a Map<Character, Boolean>

Option #1, using a Swizzle Class:

public Swizzle getSwizzle(CommandContext cmdCtx, String str) {
    return new Swizzle(ArgumentRotationAxis.a(cmdCtx, str));
}
public final class Swizzle {

    private final boolean x,y,z;

    public Swizzle(EnumSet<EnumAxis> set) {
        x = set.contains(EnumAxis.X);
        y = set.contains(EnumAxis.Y);
        z = set.contains(EnumAxis.Z);
    }

    public boolean getX() {
        return x;
    }

    public boolean getY() {
        return y;
    }

    public boolean getZ() {
        return z;
    }
}

Option #2, using a Swizzle Enum:

enum Swizzle {X, Y, Z}
public EnumSet<Swizzle> getSwizzle(CommandContext cmdCtx, String str) {

    EnumSet<EnumAxis> nms_axies = ArgumentRotationAxis.a(cmdCtx, str);
    EnumSet<Swizzle> api_axies = EnumSet.noneOf(Swizzle.class);

    if (nms_axies.contains(EnumAxis.X))
        api_axies.add(Swizzle.X);
    if (nms_axies.contains(EnumAxis.Y))
        api_axies.add(Swizzle.Y);
    if (nms_axies.contains(EnumAxis.Z))
        api_axies.add(Swizzle.Z);

    return api_axies;
}

Or Option 3, returning it as a Map:

public Map<Character, Boolean> getSwizzle(CommandContext cmdCtx, String str) {

    EnumSet<EnumAxis> nms_axies = ArgumentRotationAxis.a(cmdCtx, str);
    Map<Character, Boolean> api_axies = new HashMap();

    api_axies.put('x', nms_axies.contains(EnumAxis.X));
    api_axies.put('y', nms_axies.contains(EnumAxis.Y));
    api_axies.put('z', nms_axies.contains(EnumAxis.Z));

    return api_axies;
}

I don't think I'm forgetting anything major, but if I am, ofc correct me :P

Edit #4: In the same line of thinking above, Ranges should be roughly as easy. There would be two easy ways to add them. Via an IntegerRange (and DoubleRange) class, or via a Map<String,Integer>

Option #1:

public IntegerRange getIntegerRange(CommandContext cmdCtx, String str) {
    return new IntegerRange(ArgumentCriterionValue.b.a(cmdCtx, str)); // The `b` in the int version of the class. If it was `a`, then it would be a float
}
public final class IntegerRange {

    private final int low, high;

    public IntegerRange(CriterionConditionValue.IntegerRange range) {
        low = range.a(); //I have no idea if a is high or if a is low.
        high = range.b();
    }

    public int getLow() {
        return low;
    }

    public int getHigh() {
        return high;
    }

}

Option #2:

public Map<String, Integer> getIntegerRange(CommandContext cmdCtx, String str) {
        CriterionConditionValue.IntegerRange range_raw = ArgumentCriterionValue.b.a(cmdCtx, str);

    Map<String, Integer> range = new HashMap();
    range.put("low", range_raw.a());
    range.put("high", range_raw.b());

    return range;
}
i509VCB commented 5 years ago

One option would be to provide the NBTTag as a String within a wrapper and then let the plugin developer use something like NBT API with the provided String. Or if you want to enter the land of Optional dependancies then a hook to NBTAPI is possible.

But of course weather Jorel wants to adopt the argument is for him to decide.

JorelAli commented 5 years ago

@Minenash I think the Swizzle is something that won't ever be "as good as" the NMS implementation since it includes various other methods (such as reverse a direction, parse the directions to NESW and up and down, rotate anti-clockwise etc.), however the idea of an EnumSet is probably the best way to go about doing it.

For ranged arguments, I think having a wrapper class would be the best method, with extra methods such as "is within range" to check if a number is within a certain range.

@i509VCB I don't have a problem using optional dependencies and will consider it after adding other arguments. If anything, NBT will definitely be the last thing to add because it's a much more complicated matter than ... basically every other argument


Another thing to note is that despite some of these arguments being rather niche and unconventional, I'd appreciate examples of where you'd use each of these arguments for the purpose of documentation. I think examples are great and really help inspire and aid developers when designing their own plugins.

JorelAli commented 5 years ago

minecraft:time doesn't exist in 1.13, only 1.14 and onwards. This would make this argument ONLY available for servers running 1.14 onwards.

JorelAli commented 5 years ago

minecraft:column_pos doesn't exist in Minecraft 1.13 (but does exist in Minecraft 1.13.1+). This could circumvented by using just the vec2 regardless of whether the developer chooses BLOCK_PRECISION or PRECISE_PRECISION, but it will definitely need its own section in the documentation to state this.

Minenash commented 5 years ago
I think the Swizzle is something that won't ever be "as good as" the NMS implementation since it includes various other methods (such as reverse a direction, parse the directions to NESW and up and down, rotate anti-clockwise etc.)

Can you explain this a bit further? From the docs I can find, and from personal testing, Swizzles are a set of 3 non-duplicate axis in any order. If I type anything other than x,y, or z then minecraft gives me Invalid swizzle, expected combination of 'x', 'y', and 'z'

In your time implementation, I see this, and I'm confused on what's happening.

public Object getTime(CommandContext<?> cmdCtx, String key) {
    // TODO Auto-generated method stubreturn new CraftPotionEffectType(ArgumentMobEffect.a(cmdCtx, str));
    // ArgumentTime.a(cmdCtx, str);

    return cmdCtx.getArgument(key, Integer.class);
}

LOCATION2D, shouldn't it be LOCATION_2D?

In the original post, I had the type dimension, and it never got mentioned by either of us again. If this argument shows any loaded worlds, it would be really useful. If it's only Overworld, Nether, The End, then nevermind.

And I'll start on those examples

Edit: Shouldn't the min/max be final ?

Also, what's the reason for returning Objects instead of Location and IntegerRange ?

Minenash commented 5 years ago

-Float.MAX_VALUE wouldn't Float.MIN_VALUE be better here?

JorelAli commented 5 years ago

NO MIN_VALUE is the smallest value GREATER THAN 0. -MAX_VALUE is the SMALLEST POSSIBLE FLOAT. Giant Java misconception.

See here and here

JorelAli commented 5 years ago

If you're referring to the enum value LOCATION_2D, who cares? That's an enum used internally by the CommandAPI and not by the end user.

Time works, ignore it. They should be returning Location and IntegerRange, but it doesn't matter because of the internals of the CommandAPI. I will correct this for consistency sake.

JorelAli commented 5 years ago

My bad, confused EnumAxis with EnumDirection. Swizzle should be fine.

Minenash commented 5 years ago
NO
MIN_VALUE is the smallest value GREATER THAN 0.
-MAX_VALUE is the SMALLEST POSSIBLE FLOAT.

Oh ffs Java

JorelAli commented 5 years ago

So, I'm not sure if I want to actually call it "Swizzle" for implementation sake. I want to keep things "Bukkit" like, since that's what most developers will be comfortable with. Hence the reason that LocationArgument is called ... LocationArgument (It's an argument that returns a location, regardless of whether it's integer or floating point components).

AxisArgument?

JorelAli commented 5 years ago

Likewise with column pos, Minecraft 1.13 doesn't have minecraft:dimension, whereas 1.13.1+ does. Any ideas?

Minenash commented 5 years ago

AxisArgument sounds good. Though do you know if Location supports NaN values? If so, that might me the better y-value.

Any ideas?

As in ideas on what to do in 1.13 where it doesn't exist?

Edit: AxesArgument might be better, as there can be more than 1

JorelAli commented 5 years ago

The best thing to do would be to create a subclass of Location that throws an exception when accessing the Y value. This would make it compatible with functions that take in Location as an argument.

I have no ideas for what to do in 1.13 where it doesn't exist, other than to let the developer make up some argument on their own using the available tools (LiteralArgument is the best workaround, but harder to implement, StringArgument with suggestions of the various dimensions would be easier to implement... but eh)

Minenash commented 5 years ago

Couldn't we just not have it supported in the 1.13 build, like Throwing an Exception when it tries to be used. If you want some sort of implementation, then SuggestedString would be the way to go, though I really wish Minecraft had a ForcedSuggestedString, where you have to pick from the suggestions without making a bunch of literals

JorelAli commented 5 years ago

I too wish that ForcedSuggestedStrings were a thing. Yeah, I can add an exception no problem.

Minenash commented 5 years ago

Some basic Examples:

Rotation: /entity rotation 0 90

Location2D: /worldborder center 100 100

Axes: /flip 0 0 0 5 5 5 xy

Dimension: /tp Notch 0 100 0 survival (if if it can only be overworld/nether/end, then one of those)

Time: /protextLeverSpam 5s 15t (there's better command names, but I needed something descriptive)

Long: /storeBigValue 300000000000

Scoreboard Slot: /showStats Sidebar /showStats List

Team /tpallhere visitors

Score_Holder: If Console was added as a fake player:

/msg Notch Hi //Works
/msg Console Hi //Works
/msg rerbrweerb Hi //Would Not work 

Range:

/killAllInRange ..100
/killAllInRange 1..10

Component: Yes I know this could be done interally by having %s, %m, and %t defined, but this is just an example: `/setPMFormat {"format": "%s has messaged you at %t: %m", "variables": {"%s": "messageSender","%t": "messageTimeStamp", "%m": "message"}}

JorelAli commented 5 years ago

Shouldn't the min/max be final ?

You can't change it can you? Only with reflection, and if you're using reflection who cares if it's final?

Minenash commented 5 years ago

I guess. I'm just used to making things final when it wouldn't be changed. It also change the guaranteed behaviour in terms of cross-thread visibility: after a constructor has completed, any final fields are guaranteed to be visible in other threads immediately. Though I'm not sure anyone (including me) cares about that.

JorelAli commented 5 years ago

A note about time: It only supports either d, s, t OR no character. For example, these are accepted:

2d
10t
30s
2.3s
2

These aren't accepted:

2d10s
2s2t
Minenash commented 5 years ago

Welp, that's fine. Just not as useful

JorelAli commented 5 years ago

A note about rotation: In Bukkit, rotation is handled with the Location class. Just sayin'. It would have this weird effect of "what do we assign as the x, y, z, world values for the rest of the location class?"

Minenash commented 5 years ago

We could store it as a pair instead. The main advantage of Rotation vs vec2, is when tab-completed, it used the player rotation. So we could just return a Vec2 Argument

JorelAli commented 5 years ago

Currently, Vec2 also returns a location, specifically the Location2D class which was added in the latest commit. Either way, can totally make a new class for storing our pitch and yaw as a pair.

Minenash commented 5 years ago

Vec2 also returns a location I don't know how I forgot that in like 2 minutes... yaw/pitch class sound good.

Edit: With all of these Wrapper classes, maybe they should be put into it's own directory? Like api/wrapper

JorelAli commented 5 years ago

Totally unrelated note, but I'd love to continue working on this right now because I'm in a good coding mood, but it's 31° out here (88°F) and it's crazy.

These wrapper classes should be in their own directory, but it would have major implications on anyone who's currently using them, which would cause everything to fail and imports to be re-written.

Since this is a pretty major update, I think I can afford such a sacrifice.

JorelAli commented 5 years ago

I think I'll call it AxisArgument for the sake of confusion with axes: image

Luckily, Bukkit includes the Axis enum (declared here), which I'll use in an EnumSet

Minenash commented 5 years ago

Okay, so we only have these left:

I don't think the following would be added, but just want verification of that:

Edit:(Just for completeness sake)

JorelAli: Added item slot argument
https://github.com/JorelAli/1.13-Command-API/commit/39a9ff41a982ed7c24c5d35bad0b3b59dfc63bb4
i509VCB commented 5 years ago

88 is a good day in Texas.

Aside from the temperature jokes and climate change, I think any refactors should probably be done now.

The dimension argument is weird as bukkit adding a world includes an option add additional worlds which have their own dimension offset. This causes issue for say wanting to /execute in world2, where that would work when executed but tab completion on client would say it's invalid and the error checks are done client side and hard-coded.

JorelAli commented 5 years ago

For clarification, all that a dimension is is the "surface world", "nether" or "end". Dimensions (EnvironmentArgument) returns an Environment (Declared in Bukkit's World class), which is an enum.

I'll have to look into the various other arguments (message, resource location etc.) before making any sound conclusions.

I agree - I think I've refactored everything I want to, and am hoping I can find anything else before I push out the release after all of this is done!

JorelAli commented 5 years ago

minecraft:resource_location is one of the more complicated arguments (See the documentation section on technical arguments). For sake of complexity and the fact there's not really a Bukkit equivalent, I'll not be adding this.

minecraft:entity_anchor seems unnecessary, given that the two anchors are either feet or eyes, which is handled in the LivingEntity class. The feet height (surprise surprise) is just the same as a regular location, and the eyes height is declared here.

I'll have to look at minecraft:message in more detail.

Minenash commented 5 years ago

Well minecraft:entity_anchor would allow the user to pick which one is used for a command, so the declaration of it in a class doesn't seem relevant to yes/no to adding it. Completely fine without having it though, I can't see a real use for it.

Minenash commented 5 years ago

Sidebars can be set to teams that are a certain color, like sidebar.team.gold. I don't know how this is handled in MC though, but it is a suggestion for the ScoreboardSlot

JorelAli commented 5 years ago

This can be handled in Minecraft - it depends on whether it can be handled in Bukkit

Minenash commented 5 years ago

If it can't, then the suggestions should be overridden by the api to just include the 3.

Edit: After briefly looking into how scoreboard slots work in bukkit, (make a slot, then add players to them), then we could return the slot type sidebar, AND either a ChatColor or string of the team, so that when devs gets it, they can apply the sidebar to every one on that team.

JorelAli commented 5 years ago

Can you provide an example of how it'd look in action? From the Minecraft side of things, all the ScoreboardSlot argument returns is an integer. 0-2 represent the various displayslots.

I happen to know basically nothing about scoreboards/objectives/teams...

Minenash commented 5 years ago

okay this is a quick and dirty way to do it (took me forever to figure out), but this is the basis:

public void setTeamScoreboard(DisplaySlot slot, ChatColor color) {

    ScoreboardManager manager = Bukkit.getScoreboardManager();
    Scoreboard board = manager.getNewScoreboard();

    Objective objective = board.registerNewObjective("blues_sidebar", "dummy");
    objective.setDisplaySlot(DisplaySlot.SIDEBAR);

    Team team = board.registerNewTeam("blueTeam");

    if (slot == DisplaySlot.SIDEBAR && color == ChatColor.BLUE)
        for (OfflinePlayer player : team.getPlayers())
            if (player instanceof Player)
                ((Player)player).setScoreboard(board);
}
JorelAli commented 5 years ago

I see. After looking at this, I think I have a slightly better understanding of the display slots.

I'll temporarily put it on hold for the next hour or so, I think I've mentally cracked how to implement Teams.


Also, I'm going to struggle to come up with some way of testing these (scoreboard, teams, objectives) arguments with the CommandAPI since I don't know anything.

JorelAli commented 5 years ago

Okay, I have no idea how to implement teams.

The ArgumentScoreboardTeam argument gives me a ScoreboardTeam object. I need to somehow convert this into the Bukkit Team object. There's basically no way of doing this as far as I know without some reflection or something.

I have access to the team name, which I could use to get the scoreboard team using regular bukkit methods (Given a Bukkit Scoreboard object, I could use getTeam() to get a Bukkit Team object), however how can I ensure I have the right Scoreboard object?

All I'm seeing is reflection as the only real method here and didn't really want to go down that route... considering it should be something pretty simple, right?

Minenash commented 5 years ago

Well tmk, Normal MC only has one scoreboard, but since bukkit can have more, there's no guarantee.

However the list of teams must be coming from somewhere, and I think it's coming from what scoreboard the commandrunner has set. So assuming that's true, we could get the scoreboard of the player running the command. Testing would be needed, and I have no idea how console would work.

Another way we could do it, is on the creation of the argument, the dev passes in a scorboard, and you use that. However this might not match up with the player suggestions (or even prohibit them from sending at all) if the player is on a different scoreboard then what's given.

So the first option seems more safe and more flexible, but would definitely need testing.

JorelAli commented 5 years ago

Thing is, I have access the scoreboard (the NMS Scoreboard), so that's not a problem. I need to convert that to some sort of CraftScoreboard to actually use it however:

In order to access any of these objects, I need to use reflection and as far as I know, that's the only way to "convert" from NMS to OBC.

The only reason I'm so hesitant is that I created this fancy reflection library and I never considered private constructors, therefore there is a teeny tiny chance that the reflection could fail on some arbitrary Minecraft version.

Minenash commented 5 years ago

Would it be possible to get the CommandSender?

JorelAli commented 5 years ago

Yes. The tricky part is trying to figure out which scoreboard they're referring to.