boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
9.99k stars 704 forks source link

Plugin APIs no longer available in turn order methods #605

Closed delucis closed 4 years ago

delucis commented 4 years ago

(This is completely edited as I misdiagnosed this problem initially.)

I was using this in my initial turn order:

first: (G, ctx) => ctx.random.Die(ctx.numPlayers) - 1

But after updating to 0.39.4, the randomness API is no longer available here. (It is available during setup.) I guess this makes sense in the case of the Events API for example, but it would be nice to be able to randomise things here as was previously possible.

Original misdiagnosed issue report Previously I used `ctx.random` to randomise various bits of game state in my game’s `setup` function. (As I outlined in https://github.com/nicolodavis/boardgame.io/issues/601#issuecomment-607126633.) I’ve just realised that the new plugin code doesn’t make the plugin APIs available in setup. Can we restore that previous behaviour? Currently a plugin’s `setup` method notionally receives `G`, but none of the current plugin implementations use it, and I feel like it’s reasonable for a plugin not to depend on game-specific state at start-up. That would make it possible for the randomness API to be available as previously. There are some questions, e.g. what happens to the events API if it gets called during setup? Or should there be some way for a plug-in to indicate if it is initialised before or after game state?
nicolodavis commented 4 years ago

Ah this must have been an oversight on my part when I migrated the events and random API`s to plugins. Will take a look.

vdfdev commented 4 years ago

+1 I also could not do ctx.events.endPhase() there

delucis commented 4 years ago

@flamecoals That is related, but can’t you use endIf in your phase config instead of using an event in the turn order?

vdfdev commented 4 years ago

@delucis good point, I will try that. What I am dealing with is the turn.next on takesix code, the most popular gmae we have, returns undefined which used to finish the phase:

https://github.com/freeboardgames/FreeBoardGames.org/blob/d0904895c80d4ee55dcabee49833b4f7ba592279/src/games/takesix/game.ts#L151

But now returning undefined throws an error/warning by the framework, even though it still works: https://github.com/nicolodavis/boardgame.io/blob/45f4f432718c20a1a0f052fe0c2c25dc41e34a0b/src/core/turn-order.ts#L294

I am trying to "fix" it by replacing with equivalent behavior, hence I tried ctx.events.endPhase(), but it didnt work.

I will try your suggestion to see if I can find a criteria that fits when the turn should be ended.

delucis commented 4 years ago

Ah, I think that’s my bad, introduced in #597.

Are you sure undefined doesn’t work any more?

I added this error logging, which I guess is too strict given that returning undefined is permitted, but I didn’t actually change the logic, so perhaps it’s working as previously, just with an incorrect log message?

https://github.com/nicolodavis/boardgame.io/blob/45f4f432718c20a1a0f052fe0c2c25dc41e34a0b/src/core/turn-order.ts#L292-L296

vdfdev commented 4 years ago

right, it works, but we get a bunch of these error messages, especially on our test suite... But it still works :).

I was just worried that it meant that in the future it wouldnt work anymore, so I want to do it the "right" way, but there is no way to just use the drop-in replacement of ctx.events.endPhase()...

vdfdev commented 4 years ago

I will try to see if endIf works though :)

delucis commented 4 years ago

I’ll fix that error message. It was mainly intended to flag errors when people returned a string instead of a number (confusion between play order position and player id).

Returning undefined is how some of the default turn orders work, so I don’t think that will go away any time soon!

vdfdev commented 4 years ago

Cool, thank you!

nicolodavis commented 4 years ago

This is fixed in https://github.com/nicolodavis/boardgame.io/commit/eb1e06070eabe7d2e723b3f35fd553100183621a.

nicolodavis commented 4 years ago

The change makes all plugins available in the turn order functions, but we should restrict it to just the "random" plugin (it would be somewhat surprising if events could be generated from an object that is merely describing the turn order).

delucis commented 4 years ago

Fantastic!

Could there be a plugin option that restricts its availability (or the opposite)? I guess in general there might need to be a slightly more systematic approach to which plugins work where, but the obvious distinction is that events should only be allowed in hooks that are expected to mutate state, so not in endIf or turn, but only the onX hooks. Does that sound right? Maybe an option like allowInNonMutatingHooks to enable things outside the onX hooks?

nicolodavis commented 4 years ago

I was thinking of just passing a list of plugin names to EnhanceCtx that can be used to control what plugins get added. Preset lists of plugins that need to be available at different places can be exported as constants from that file.

delucis commented 4 years ago

How would that work for third-party plugins? Would they have to export a list of these places too? Or would they be available everywhere with no configuration options?