ComputerCraft icon indicating copy to clipboard operation
ComputerCraft copied to clipboard

Command Event

Open dirthsj opened this issue 8 years ago • 8 comments

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!

dirthsj avatar Jul 09 '17 03:07 dirthsj

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.

SquidDev avatar Jul 09 '17 06:07 SquidDev

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?

BombBloke avatar Jul 09 '17 07:07 BombBloke

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

KnightMiner avatar Jul 09 '17 14:07 KnightMiner

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.

SquidDev avatar Jul 09 '17 14:07 SquidDev

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.

KnightMiner avatar Jul 09 '17 15:07 KnightMiner

@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.

dirthsj avatar Jul 09 '17 15:07 dirthsj

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

SquidDev avatar Sep 10 '17 12:09 SquidDev

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

dirthsj avatar Dec 03 '17 04:12 dirthsj