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

Command Event #362

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@dirthsj
Contributor

dirthsj commented Jul 9, 2017

Adds a /computer <id> <msg> command, which queues a computer_command <msg> event on the correct computer, if possible.

This allows certain things like text and book interfaces with command computers.

I will gladly accept modifications restricting this to command computers only; I can't figure out how to make that work. Done!

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jul 9, 2017

Just a couple of suggestions:

  • Instead of storing a list of all computers and looping through them, just use ComputerCraft.serverComputerRegistry.lookup. This means you won't get memory leaks with the Computer.
  • ServerComputers receive the ComputerFamily in the constructor. If you save this as a field and provide a getter, you can do ensure this only works for command computers.
  • It looks like you support passing multiple arguments in the computer event, so it might be worth adjusting the documentation to show that - I think Minecraft uses [arg]... but I'll double check.
  • Integer.valueOf will throw an ugly exception if the ID isn't a number. You should catch the NumberFormatException and throw a CommandException.
  • Similarly, instead of printing an error message when the computer cannot be found, throw a CommandException. I'd also try to avoid printing a success message - I think it'll show up when used from a book, etc... and so could be a little ugly.
  • I feel the computer event name is a bit too general - I'd stick with command_computer.
  • The command should probably override CommandBase instead of implementing ICommand - this way you get sensible defaults for methods like compareTo - your current implementation will possibly break sorting in the /help command.
  • Maybe include the username and UUID (if available, obviously not if a command block) in the event - this way you can tell who pressed it.

I can definitely see this being useful, much better than polling things every tick. Looking forward to using it in Taken.

@BombBloke

This comment has been minimized.

Contributor

BombBloke commented Jul 9, 2017

This sort of strikes me as being half a chat box (as implemented by various add-on mods since MiscPeripherals) - it'd be great to have this functionality regardless, but would it maybe be better to add an official version of that classic peripheral at last?

@KnightMiner

This comment has been minimized.

KnightMiner commented Jul 9, 2017

One issue I see with this is a lack of a permissions check meaning it makes using a pocket computer as a remote less useful as it's harder to implement/more expensive. I think it would be good for command computers only, but make the ID optional letting the computer decide to handle it as otherwise you are telling users on your server to add an arbitrary number before commands which is never good to remember.


I have been thinking about how to bring chatboxes to cc vanilla in a way that prevents discouraging pocket computers. A few notes I have on how I would implement it:

  • Always have a range limitation on the chatbox. For a base that is chat controlled, these can be periodically placed for full coverage (might want some message internal ID so the computer can check for duplicates)
  • To still allow freedom, add a pocket variant that can receive messages while it's in the players inventory. The messages can be redirected using rednet to the intended computer or just run on the pocket computers itself (as a Siri like system. We will want a second peripheral on command computers so they can have both a modem and a chatbox then.
  • For the sake of servers, there will be a admin/command version that cannot be crafted but has no range limitation. This allows censoring/server commands/whatever else.
  • I am not sure if sending chat messages as part of this makes more sense or if it would be better to move that to the speaker. Maybe rename the chatbox to a microphone, and add sound event detection if it can be done cleanly/efficiently
@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jul 9, 2017

Make the ID optional letting the computer decide to handle it as otherwise you are telling users on your server to add an arbitrary number before commands which is never good to remember.

@KnightMiner I got the impression from the PR that this is designed to be an "invisible" command, much like vanilla's /trigger. It would only be used inside clickEvents (such as the output of /tellraw or in written books) in order to communicate with a command computer behind the scenes.

An example would be in Taken - I have the ability to switch the player into a "spectator mode", teleporting them back to where they started when they press a button. This would normally be implemented by the /trigger command and polling the scoreboard every tick. Here we could just wait for an event.

Chat boxes would be also nice, but do serve a separate purpose - they designed to be used by the average user for more "public" interactions with a computer.

@KnightMiner

This comment has been minimized.

KnightMiner commented Jul 9, 2017

Fair enough, though you would still have to ensure your computers ID does not change or all your tellraw needs to be updated. Though, you could probably use a local variable if all commands are run in one computer I guess.

@dirthsj

This comment has been minimized.

Contributor

dirthsj commented Jul 9, 2017

@SquidDev I've implemented everything in your first post
@BombBloke @KnightMiner Yeah this is pretty much a replacement for /trigger like @SquidDev said
I did figure out the appropriate permissions level for that now.

}
try {
ServerComputer computer = ComputerCraft.serverComputerRegistry.lookup(Integer.valueOf(args[0]));
if( computer.getFamily().equals( ComputerFamily.Command ) ){

This comment has been minimized.

@SquidDev

SquidDev Jul 9, 2017

Contributor

This can just be == as it's an enum. Also worth noting that .lookup may return null, so you should probably have a null check here too.

This comment has been minimized.

@dirthsj

dirthsj Jul 9, 2017

Contributor

Done :)

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Sep 10, 2017

@KingofGamesYami This'll need rebasing onto master: it's been updated to 1.11.2 which introduces some incompatibilities.

throw new CommandException( "Usage: /computer <id> <value1> [value2]..." );
}
try {
ServerComputer computer = ComputerCraft.serverComputerRegistry.lookup(Integer.valueOf(args[0]));

This comment has been minimized.

@SquidDev

SquidDev Nov 23, 2017

Contributor

Having another think about this, I think it would be better to do find all computers with the given ID, queuing for those which are commands. If no such computers are found then it'd error. Most of the time they'll only be one computer with the given ID, but some programs (cough Taken cough) spawn multiple computers with the same ID which makes this unusable.

@dirthsj

This comment has been minimized.

Contributor

dirthsj commented Dec 3, 2017

@SquidDev I finally got around to writing and testing a fix. Sorry about the wait, I've been busy with college.

for( ServerComputer computer : ComputerCraft.serverComputerRegistry.getComputers() ){
if( computer.getID() == id && computer.getFamily() == ComputerFamily.Command ){
computer.queueEvent("computer_command", ArrayUtils.remove(args, 0));
found_valid_computer = true;

This comment has been minimized.

@Lignum

Lignum Dec 3, 2017

Contributor

There shouldn't be any computers with duplicate ids, so add a break here? Or, since we now have Java 8, if we make that assumption, this can be a whole lot nicer:

Optional<ServerComputer> optServerComp = ComputerCraft.serverComputerRegistry.getComputers().stream()
    .filter(c -> c.getID() == id)
    .filter(c -> c.getFamily() == ComputerFamily.Command)
    .findFirst();

if(optServerComp.isPresent())
{
    optServerComp.get().queueEvent("computer_command", ArrayUtils.remove(args, 0));
}
else
{
    throw new CommandException( "Computer #" + args[0] + " is not a Command Computer" );
}

This comment has been minimized.

@dirthsj

dirthsj Dec 3, 2017

Contributor

Did you read anything in the comments and reviews? I previously supported 1 computer instance, and now support more - at SquidDev's request. It is definitely possible to have more than 1 computer with the same ID.

This comment has been minimized.

@SquidDev

SquidDev Dec 3, 2017

Contributor

In fairness, that review is now hidden as you've made changes. Sometimes GH's features are counter-productive :/.

@Lignum For reference, this was requested because Taken spawns pocket computers with the same ID as a command computer, meaning in its original form the command failed.

This comment has been minimized.

@Lignum

Lignum Dec 3, 2017

Contributor

Hmm, alright, in that case a Stream won't be any nicer than what we have now. Nevermind then.

This comment has been minimized.

@dmarcuse

dmarcuse Dec 3, 2017

Contributor

It can still be done for multiple computers with a Stream, in a way that I think is even nicer:

ComputerCraft.serverComputerRegistry.getComputers().stream()
    .filter(c -> c.getID() == id)
    .filter(c -> c.getFamily() == ComputerFamily.Command)
    .peek(c -> c.queueEvent("computer_command", ArrayUtils.remove(args, 0))
    .findFirst().orElseThrow(() -> new CommandException("No command computer with ID " + args[0] + " found"));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment