Minor fixes to CombatController

[devpitOne]

Fixes for:
-teamCount is never incremented
-Flee causes enemy mobs to be removed
-Combat Messages displaying after the look command from combatEnd

[seanohue]

Overall good! A small formatting nitpick. Also I'm going to test this out tonight but I trust that it works.

[seanohue]

JS Formatting: I would put the brace immediately after else, like else {, then the comment.

[devpitOne]

Modified to match the recommended format(5c344ea)

[seanohue]

Interesting approach here. Why not just emit the combat end event directly?

[seanohue]

Ah, I see this must be the fix for the extra combat messages. I'd put a comment here explaining that. Seems like an odd side effect but glad you figured this one out.

[devpitOne]

Note that this occurs on a ranvier using the standard bundles with turn-combat added. It might not be occurring on a ranvier using the full trpg-skeleton

[devpitOne]

The Bugfix for the attack command(ee9bd7b) will not work without a core change to Character.isInCombat()
My modified core adds a check for the CombatController.
Shawn recommends not using isInCombat at all. Instead the check could be changed to whether the target.combatController exists.

It shouldn't break anything as for trpg-skeleton that method will always return false, meaning the existing logic will happen.

[seanohue]

My workaround for that was to add a method, Combat.isInCombat, to the combat library (mostly custom but similar to the one found in the round-based combat bundle). That method checks for the combat controller.

[seanohue]

For the sake of keeping PRs small I would consider adding this fix as a separate PR particularly as it has several TODOs. Smaller PRs mean a higher chance for each individual one to get merged in sooner than later.

[devpitOne]

Agreed, I just found another bug with it, so I'll back this one out of the PR

[azigler]

These changes work great on my end. Good catch with adding this._teamCount[team]++; to addParticipant because otherwise those counts stay at 0. Removing Broadcast.prompt(p.character); gets rid of that duplicate prompt after a battle.

I'm not sure I understand why you're queuing a player emit in the command queue, however. combatEnd actually does nothing at the moment, but even if it did it wouldn't belong in the command queue. Why not fire it immediately?

[devpitOne]

Hiya, It's to do with the order of commands. Calling it directly caused anything in combatEnd to occur before all the endCombat() was complete. This seems to be the wrong way round to me.

[azigler]

I agree with that idea. The issue with this implementation, however, is the player events for commandQueued and updateTick are missing in this repo, so there's actually no functionality to execute the queued command. As a result, it would never fire.

You could solve this by adding p.character.commandQueue.execute(); at the end of that for loop block, but manually executing the command queue is messy. Alternatively, you could add the appropriate event handlers to your player-events.js in order to make the command queue function:

commandQueued: state => function (commandIndex) {
  const command = this.commandQueue.queue[commandIndex];
  const ttr = this.commandQueue.getTimeTilRun(commandIndex);
},

updateTick: state => function () {
  if (this.commandQueue.hasPending && this.commandQueue.lagRemaining <= 0) {
    B.sayAt(this);
    this.commandQueue.execute();
    B.prompt(this);
  }
}

And if you're going to be editing the player events, you might as well also add a handler for combatEnd since that's currently missing. I would do something like:

combatEnd: state => function () {
  Broadcast.sayAt(this, 'You leave combat.')
}