PrismarineJS / flying-squid

Create Minecraft servers with a powerful, stable, and high level JavaScript API.
https://prismarinejs.github.io/flying-squid/
MIT License
547 stars 99 forks source link

Separate emit and on in behaviors #132

Open rom1504 opened 8 years ago

rom1504 commented 8 years ago

Currently there are stuff like this:

player.behavior('punch', {}, () => {
      player._writeOthersNearby("animation", {
        entityId: player.id,
        animation: 0
      });
    })

And this is really hard to think about because it is both emitting events and listening on it (default action).

It would probably be simpler to do

player.emitBehavior('punch', {});
player.on('punch',(data,cancelled) => {
  if(cancelled) return;
  player._writeOthersNearby("animation", {
        entityId: player.id,
        animation: 0
      });
});

Or even just:

player.emitBehavior('punch', {});
player.on('punch',(data) => {
  player._writeOthersNearby("animation", {
        entityId: player.id,
        animation: 0
      });
});

I wonder why the 'punch' event is emitted at all if it has been cancelled ?

Just some possibilities, but the current "default action" thingy is very confusing imho (why is there only one default action unlike normal events ?)

rom1504 commented 8 years ago

On the same theme, stuff like https://github.com/mhsjlw/flying-squid/blob/5bff660bbea5cee64c00cabd354edf598305f2cb/src/lib/plugins/pvp.js#L15 really don't make any sense. that thingy with all the parameters should be the attackEntity function, the behavior should be emitted on 'use_entity', not when that function is called.

Do you really want to enable plugins to break functions all around the project ?

edit: yeah indeed I thing "default actions" just make things confusing. (people are going to look at the code and think "I need to add stuff to the default action", instead of "I need to listen to the event and do something")

demipixel commented 8 years ago

So a few points to make:

For example, if you wanted to make a plugin that digged blocks go flying in the air, you might do:

player.on('dug', data => {
  data.blockDropVelocity = new Vec3(Math.random()*10, 10 + Math.random()*10, Math.random()*10);
});

However, if the default listener was created before this plugin was loaded (which "digging.js" comes before "external.js" so it's likely), that means that the default action gets called before that plugin. The plugin completely breaks.

People don't need to use cancels at all if they're not interested, and they don't even have to be aware that behaviors emit more than one type of event. However, it's important to note that if you've been on any server with lots of plugins (advanced survival servers, minigame servers, etc), plugins have an intense usage of "cancelling" things.

Sorry that was a lot :P

rom1504 commented 8 years ago

'However, if the default listener was created before this plugin was loaded (which "digging.js" comes before "external.js" so it's likely), that means that the default action gets called before that plugin. ' no

if you do player.on("some_event", () => { player.someNotYetDefinedFunction(); }); that won't be a problem if player.someNotYetDefinedFunction has be defined when some_event is actually emitted. (that's why plugin loading order doesn't matter)

I also have no problem with cancel, but what is the meaning of the punch event if it has been cancelled ? do people want to do things on "punch" + cancelled==true ?

demipixel commented 8 years ago

I'm having trouble thinking of examples where you would say if (cancelled) however there are plenty of situations where you wouldn't check if cancelled was true or not. For example, if you wanted a chat log plugin, you would include everything, cancelled or otherwise (in most cases). In other cases, people might use "cancelCount" over "cancelled".

But yeah, I see what you mean by "punch" + cancelled==true not really being used (except in really stupid circumstances for testing or whatever)

rom1504 commented 8 years ago

Hmm hmm yeah, there is an ambiguity between real behavior and "information behavior" like chat.

demipixel commented 8 years ago

Well, chat can actually be cancelled (won't broadcast). By "informational" behavior, I assume you mean it provides information that can be changed (if it can't be changed, it's just an event). As of right now, the only behavior that has no effect on its default action nor cancel action is "forceCancelDig". In reality, though, that should be changed (remove "stop", and then just make cancel() set stop to false).

rom1504 commented 8 years ago

related

behavior shouldn't be in functions, so this is wrong https://github.com/mhsjlw/flying-squid/blob/master/src/lib/plugins/blocks.js#L16

behavior should be between an event and a function call. Not in the function call.

behavior shouldn't break functions.

demipixel commented 8 years ago

The reason for it is for stuff like my "allglass plugin" that intervenes both sendBlock and sendChunk. Idk how else you'd do it, especially if you want to intervene another plugin's sendBlock. On the other hand, I'm mainly using the behavior to change data, not to cancel the behavior. On the other hand, you might be able to do some fun stuff cancelling a sendBlock...

rom1504 commented 8 years ago

yes I see what you mean.

But I really dislike that anything in the code, in any plugin, can break sendBlock. Maybe there should be a sendBlock and a cSendBlock (c for cancellable) that would be the behavior and use sendBlock as default function.

demipixel commented 8 years ago

Might be better to have a ucSendBlock (uncancellable). You also have to consider that stuff might break if another plugin can't intervene a sendBlock, so it's better if the plugin that failed to use ucSendBlock (if they needed it) is the one that breaks rather than the one that intervened (nothing wrong with that)

rom1504 commented 8 years ago

yeah I'm okay with that

demipixel commented 8 years ago

Better yet make "noCancel" or "uncancellable" an option.