Remove the CommandSource in favour of Causes.

[dualspiral]

This is marked as a Draft PR so that the extent of the changes I'm proposing can be seen. It is intended as a way to foster discussion on a specific point that previous PRs could not.

We are investigating how best to utilise the Cause system in our Command framework - the code in this PR is the purest option - but hear me out as I explain what we're really looking at. This is long, but please read all of it before passing comment because I'm more interested in reaching convergence of ideas rather than a "I don't like this, put it back how it was."

A recap of Causes (and why we all should love them!)

The SpongeAPI and its implementation is a system driven by Causes. They have primarily been used in events, though they have crept into data registration and other uses where we determine which plugin is doing what. It is a powerful tool in the plugin developer's toolkit, enabling not only the determination of redstone being caused by redstone, but being able to track the player who spawned the sheep who aimlessly walked onto a pressure plate turning on redstone wire and causing an explosion, or opening a door, or firing a command block...

...except actually, we don't fully expose the cause to commands (we could get the cause from the CSM, but that's a live snapshot and not the cause at the specific time a command was sent). So, we are setting out to remedy this. If a player fires a command block because they stood on a pressure plate, we want to be able to directly tell a command consumer "hey, this player was the one who set your command off on this command block. Maybe you want to target them?"

This could be really handy for game servers who use command blocks or signs to trigger events, but want the context of who triggered it, rather than who is nearby. So, we have been talking a lot internally (and in previous command PRs), but I wanted to really get opnions on this point, rather than as part of a big API PR because this is rather important.

Causing those Commands

The basic idea is that you could check the Cause for what's going on. The root might be a sign, but you can the scan the cause and find a Player who clicked on the sign. That sign could be a Subject, so permission checks are performed on that sign - but the player is the first MessageReceiver - so you send the messages to them! Similarly, you might have a command block executed by a sheep on a pressure plate - but you want to execute the command on a player only. So, no problem, let's just find the first Entity on the stack and just not execute if it's a player. By opening causes to the command system, commands can take advantage of this rich, contextual system we already have and make Sponge an even better system to develop commands against.

Even better, we could expose our Event filters to Commands, so we can reduce the amount of boilerplate you write - want to have a command that only executes if the root of the command is a Player? Perhaps two different logic paths if you have a root Player or a root Console? While this is a little later down the line - Causes will really enable all this, allowing command executors to get the same treatments as events (though specialised).

Removing those Sources?

Of course, that shouldn't all be too contriversial... but you may have been alarmed by the title of this PR. Yes, we are considering the removal of CommandSources in the API. The simple reason is, the cause already tells you the source of the command - particularly the root of the cause. The CommandSource is in API 7 a collection of the following:

• Subject - an object that permissions can be attached to;
• MessageReceiver - an object that can receive Text messages;
• A getLocale() method (moved to MessageReceiver in API 8 as that is a more logical place for it
• A getName() method for identifying this source as a String - this has been moved to NamedIdentifiable (but that name sucks IMO and could do with a better one if this goes ahead)

So, in most cases, these interfaces will simply replace the CommandSource and the various sub sources that existed prior to this. However, there are some sub sources that have already disappeared:

• ProxySource: this is a source that wraps around a source to intercept permission checks etc. A common use for it should be to front a player when executing a command from a different source, such as a Discord bot. In future, the Discord bot plugin should already be in the cause, but can push a Player to the cause prior to execution (or the player an then some other proxy Subject if they want to control permission checks, for example).
• RemoteSource didn't belong - the command system in itself shouldn't care if a command executor is remote.
• RconSource is now an RconConnection in the appropriate package
• SignSource is now just the Sign that would be pushed to the Cause, and would be a Subject only.
• CommandBlockSource similarly is just the CommandBlock and MinecartCommandBlock

Cause and Effect (on Plugin Developers)

You may see a problem here - plugin developers will still likely want to get a traditional command source out of the cause - such that they get an easy object to obtain the console or the player out of. So, we might want to consider a CommandTarget interface that can be used to group the Console, Rcon and Players together - or more precisely, interfaces that indicate objects that would operate with commands more interactively and would normally be the recipient of messages. Of course, command blocks would probably have to be in that category too as the output of command are important to them.

We also have to think about how Sponge managed components (parameters etc.) handle the cause for permissions and messages, but I was likely to go with first Subject and first MessageReceiver (there are currently overrides in the Context, but I fear they would be ignored and we are better just focusing on the direct Cause).

There is the potential to be more confusing - but with good documentation and established standards we can absolutely make the Cause the new CommandSource. We just need to make sure we do this right so it works for everyone.

Sourcing those Opinions!

So, the question is, do we:

• Just remove all CommandSources like this and document that permission checks should be performed on the first Subject, similarly, messages should go to the first MessageReceiver?
• Add a CommandTarget marker interface that performs a "quasi-CommandSource" like job, but this rename indicating that command sources are no longer actual sources (that's the root of the Cause now!)
• Always make CommandSources the ultimate root of an object (this is closest to what Mojang does but we don't necessarily need to follow that for Sponge plugins)

Open for discussion, but as always, please keep it civil and please give reasoning. Note that we will likely go somewhere between the first two points as it stands.

---

Just on the note of the Console object appearing, it's there simply because I'd completely forgotten that I'd started working on it until most of the way through this PR. Note that the Server and Console objects are both Subjects to account for the possibility that (as said elsewhere) one console could support multiple servers and so someone, somewhere, might have different permissions per server... but that could also be too complex for the API. That, however, will likely go into api-8 sooner, once @Zidane, @Cybermaxke and I have settled on this (though I doubt we're far off that).

[ItsDoot]

I vote for the first option of selecting the first Subject and first MessageReceiver.

1. It's a relatively simple solution to describe to developers new to Sponge.
2. Works across any command framework one may use, be it the one Sponge provides or a third-party library.
3. It unifies the command system with the event system.
4. It just feels right.

For a while I was really adamant against replacing CommandSource with Cause, but after some thought it actually seems like the best way going forward.

[JBYoshi]

I might be misunderstanding how this will work, but I see a few problems with simply selecting the first Subject or MessageReceiver.

Say that a player steps on a pressure plate that triggers a command block. I'm assuming the player would be earlier in the cause (because the pressure plate processing occurs before the command block processing).

• This would mean that a plugin grabbing the first MessageReceiver would get the player, and output messages would then go to the player. Normally, these messages would be stored in the command block instead of being sent to the player.
• If a plugin selects the first Subject, it would check the permissions of the player instead of the command block. The command execution should usually have the permissions of the command block, but it gets the player instead - who may have fewer permissions. (This case would also occur if a player clicks a sign and ends up before the sign in the cause.)

On the other hand, selecting the last one would also break in some cases. Take /execute as an example. Say that the console types /execute as Steve run somecommand (or /execute Steve ~ ~ ~ somecommand in 1.12 notation). The command should check the permissions of the console. However, if the console is before the player, selecting the last Subject would return the player, which would break the permission test.

Is there something I'm missing here?

[dualspiral]

Remember causes are last in, first out.

This would mean that a plugin grabbing the first MessageReceiver would get the player, and output messages would then go to the player

If a plugin selects the first Subject, it would check the permissions of the player instead of the command block. The command execution should usually have the permissions of the command block, but it gets the player instead - who may have fewer permissions. (This case would also occur if a player clicks a sign and ends up before the sign in the cause.)

No, the first MessageReceiver and Subject would be the CommandBlock as the cause is effectively a last in, first out - @First really means latest on the stack. So, when the command block is triggered, that command block should be the root and the "first" subject/receiver.

The command should check the permissions of the console. However, if the console is before the player, selecting the last Subject would return the player, which would break the permission test.

This is the tricky part. The original solution was to use the EventContext as an override (so, if there is an object in EventContextKeys.SUBJECT, that would be checked as opposed to the direct cause). However, I explain above:

We also have to think about how Sponge managed components (parameters etc.) handle the cause for permissions and messages, but I was likely to go with first Subject and first MessageReceiver (there are currently overrides in the Context, but I fear they would be ignored and we are better just focusing on the direct Cause).

In this case, there would be a specialist Subject that contained the permissions of the super user on the top of the stack, but the player second in the stack. This would allow the first Subject to be super users permissions, and the first MessageReceiver to be the player.

Now, this is why I want to use event contexts as part of our command structure! The player isn't a direct cause, and so this is part of the event context - but if we do that, plugin developers aren't going to want to check the context before the cause each time. As I write this, I wonder if we'd be better served by creating a CommandCause object that has getCause(), getSubject() and getMessageReceiver() methods on it so that most plugins can just use the subject and message receiver easily, but have access to the Cause (which is all controlling this anyway).

Of worthy note, if we were to retain CommandSource, then we'd still have to use a special proxy source to mask the player permissions when using /execute, so this is not a problem that has been created by making this switch - we would still have to make a subject proxy.

[ejm]

As I write this, I wonder if we'd be better served by creating a CommandCause object that has getCause(), getSubject() and getMessageReceiver() methods on it so that most plugins can just use the subject and message receiver easily, but have access to the Cause (which is all controlling this anyway).

So, if I'm understanding this correctly, a CommandCause would be effectively the drop-in replacement for a CommandSource? I don't see why this is any better than just having Location, Rotation, Subject, etc. each stored as an EventContextKey? Yes, plugin developers would now need to check those, but, if you have mappings to them in the existing CommandContext (which already has methods for a lot of these things already), you wouldn't need to introduce a new dummy Cause. I don't think it is too much to ask from developers to utilize the CommandContext.

I understand the challenges in that, though. It is tempting to just grab the first Player off the Cause stack and run with it, but, commands aren't that simple, especially with /execute being as powerful as it is. Plugin developers will need to keep it in mind no matter what or risk their commands having unexpected behaviors, so, using CommandContext to check for permissions, location, etc. doesn't seem like a huge ask.

Of worthy note, if we were to retain CommandSource, then we'd still have to use a special proxy source to mask the player permissions when using /execute, so this is not a problem that has been created by making this switch - we would still have to make a subject proxy.

One of the developers on a project I work on currently does this using delegation, and, it almost works, but, Vanilla commands deal with CommandSources a little funny so it causes weird side effects with Vanilla permissions. No matter what ends up happening, could this possibly be rectified?

[dualspiral]

I don't think it is too much to ask from developers to utilize the CommandContext.

Plugin developers will need to keep it in mind no matter what or risk their commands having unexpected behaviors, so, using CommandContext to check for permissions, location, etc. doesn't seem like a huge ask.

I think you're assuming that command consumers will always have a CommandContext. They won't - if you implement a command without using our framework, you simply get the Cause, not this context. That's why I'm proposing a CommandCause, it will effectively be a wrapper around the Cause with those methods on the Context. Everything will still be controlled by the Cause itself - the CommandCause would not be a dummy, but informed by (and exposes) the cause itself.

Effectively, I'm proposing that this line:

SpongeAPI/src/main/java/org/spongepowered/api/command/Command.java

Line 122 in 0520f48

CommandResult process(Cause cause, String arguments) throws CommandException;

becomes

CommandResult process(CommandCause cause, String arguments) throws CommandException; 

where CommandCause will have a getCause() method, plus the utility methods on CommandContext. As CommandContext already contains the methods, that wouldn't change on the context. It's simply a way to ensure that these methods are always available.

As for vanilla commands, yes, I'll keep this in mind.

[ejm]

I was just writing a reply after I remembered that Command doesn't necessarily need to use CommandContext. CommandCause makes sense in that case! Disregard my above comment aside from the vanilla-related concern. Thanks!

[pie-flavor]

+1 for the CommandCause idea. One of the primary failings of a map-and-key system is that what there is supposed to be is not immediately clear; which ones are optional in a particular context vs which ones are just never present in that context, for example. EventContextKeys should still be accepted in that cause, but a wrapper API guaranteeing things like location of execution, dimension of execution, alignment, rotation, executor, original executor, should have dedicated methods in some kind of wrapper class, that don't return Optional, as guarantees that these things always exist. Especially because these things are only used in commands and nowhere else, and context keys used lots of other places won't be used here.

[liach]

Soon:tm: sponge will be the plugin api that can detect the malicious plugin that called "player.setOp" :laughing:

Note for implementation: While most parts of minecraft can be simulated by the vm stack, the command functions are stored in an external stack and their cause, etc. needs to be handled delicately.

[dualspiral]

Any more comments? I'm going to merge this soon otherwise (and fix a couple of javadocs that have TODOs on them).

[pie-flavor]

We have the location but not the rotation of the command. Maybe change the Location to a Transform?

[dualspiral]

How does that work if a block is the focus of the command?

[pie-flavor]

It would face the direction the block is facing (since command blocks are directional). Or maybe it'd be a rotation of zero if it's a nondirectional block, I don't know. But commands can have relative rotations to the execution targets, so a. it's gotta be in there and b. vanilla knows what to do.

[liach]

Making sure that all quantities in vanilla ServerCommandSource is present in the cause stack, and we should be fine.

[dualspiral]

Or maybe it'd be a rotation of zero if it's a nondirectional block, I don't know.

And this is why it shouldn’t be a Transform, there is a difference between 0,0,0 and no rotational data. Adding a rotation separately makes more sense in this case.

I do have to point out that this is all driven by the Cause and the CommandCause is solely driven by that. There is no separate cause stack for a command in normal usage. I’m not keen on adding every helper method under the sun, in the example of a command block, it’s going to be the message receiver so you can always get more information from that. Similarly, we do have the target block method which would also have this info.

I am not saying I’m against adding the rotation, but do remember that the CommandCause is to simplify common actions and to allow plugins to harmonise with the impl (what Subject is being checked, etc.) and is not intended as a draconian toolkit that removes the point of the Cause in the first place.

Plugins are allowed to think for themselves too!

[pie-flavor]

Right. The point of a CommandCause is to provide stuff that's guaranteed to be relevant.
The reason, however, that you can't just get the rotational data from the command block, is because commands have a relative rotation just like they have a relative position that may or may not be the execution target's actual position or rotation.

[dualspiral]

The point of a CommandCause is to provide stuff that's guaranteed to be relevant.

Where did I say that? Also what determines what is relevant? The Cause contains all information. The CommandCause contains that Cause and helper methods. That's it.

Very much like how events don't have any particular guarantees, we've made that the case with commands too by making it use the Cause system. The point is that command can see exactly where the command came from, who executed, why that execution occured, etc. etc. It all goes on the Cause and the CommandCause is a simple wrapper for the Cause that explains how the implementation will select a Subject, for example, so that plugins don't need to check the SUBJECT context on the cause before looking for the first Subject in the Cause. Same with the MessageReceiver.

Quite simply, the point of the CommandCause is to harmonise how the Cause is used across Sponge and plugins, particularly across obtaining the Subject and MessageReceivers.

The Cause is the ultimate control of this process. Everything will be present in the Cause, the CommandCause has no other information beyond this. This wrapper can simply be thought of as a specialised "event". That's it.

[pie-flavor]

Exactly. And the event has a rotation associated with it.

[dualspiral]

Okay. I don't really think you're getting this point of this entire command system here and you are fixated on one point that I've not even rejected.

Commands in Sponge can be summed up like this:

Commands using the Sponge system are different to those using the Vanilla system. By using the Cause system we, in effect, state that our commands have a different set of criteria upon execution - which is entirely our right to do.

Originally, after much discussion with @Zidane, I was just going to provide the Cause of the command and tell developers to go nuts. We make absolutely no guarantees about anything when a Sponge command is invoked. The is no requirement for a CommandSource (indeed, they won't exist in our system), no requirement for a Subject to be present, or a MessageReceiver, it could just be a block and a sheep for all we care. The CommandCause is simply a set of methods that allow plugins to use the same objects that Sponge selects for the subject of a command, or the message receiver. It's not a guarantee - in fact, it fills two gaps - what we do if there is no Subject or MessageReceiver.

We make no guarantees about anything, because Sponge commands have no guarantees about how they are executed. Now, if someone uses the /execute command to build up a source that has a rotation, then we'll add that to our cause. Yes, I will likely add an Optional<Vector3d> (even though the rotation is a Vec2f in the mojang code) that adds a rotation - again, not something I ever said I wouldn't do. In fact, using /execute will likely end up filling the EventContext with keys such as ROTATION.

Should Rotation be on the CommandCause? Yes, considering Location is on there it makes sense and it is something I missed. I never said that I wouldn't do it, I just haven't had enough time with all my other personal commitments to actually do it. But it seems to me that you've misunderstood what this system really is getting at - if you want something, go get it from the Cause because that really has all the information you need.

Of course, as a quasi-event, I'd be following what #1975 does.