Can't change combatant order without updating combat turn

[pafvel]

What happened?

In Foundry v12 it was possible to update the combatant order in the combat tracker without changing the index of the current turn by calling Combat.updateEmbeddedDocuments('Combatant', updates, { turnEvents: false }); This functionality is used by the Year Zero Engine: Combat module when swapping place in the combat tracker between two combatants.

In v13, this no longer works due to Combatant._preUpdateOperation adding a combatTurn to the operation.

I was able to get the same behaviour as before by adding an override in a subclass to Combatant:

static async _preUpdateOperation(_documents, operation, _user) {
    const removeCombatTurn = operation.turnEvents === false && operation.combatTurn === undefined;
    await super._preUpdateOperation(_documents, operation, _user);
    if (removeCombatTurn) {
      delete operation.combatTurn;
    }
  }

I reported a similar issue in the past. See #11531

What ways of accessing Foundry can you encounter this issue in?

• Native App (Electron)
• Chrome
• Firefox
• Safari
• Other

Reproduction Steps

Foundry v 13.345
Year Zero Engine: Combat v1.3.5
Tested in Dragonbane, but should work in the same way in any system.

Repro steps
Combat Tracker:
1 Fighter << Current
2 Thief
3 Mage

Right-click Fighter and Swap Initiative with Mage
Note: if the swap initiative menu item is missing, press F5 to re-load (bugfix for this is on its way)
Result:
1 Mage
2 Thief
3 Fighter << Current

Expected Result:
1 Mage << Current
2 Thief
3 Fighter

What core version are you reporting this for?

13.345

Relevant log output

Bug Checklist

• The issue occurs while all Modules are disabled

[aaclayton]

Hey @pafvel I might need a little more perspective on why you think core should handle it a different way. It seems to me that your new solution of having a _preUpdateOperation override on your system Combatant class is the correct solution.

The core behavior attempts to preserve the same combatant as the current turn when the array of combatants changes - which seems to me to be the correct thing to do by default unless you have different information.

In short it seems to me that this is "working as intended", but I'd like to hear your perspective on why you think a different core behavior would be more correct - and if so what that different core behavior would be.

[pafvel]

Consider the implementation of Swap Initiative between combatants, which must preserve combatTurn when one of the swapped combatants is the current (otherwise combatants may be skipped or have their turn twice). It just seems like the simplest solution, and even seems like the intended solution when looking at the code.

When reading the code, it seems to me that combatTurn should not change when operation.turnEvents === false when updating combatants. The turnEvents flag can be set when updating combatants and is undefined by default, which

1. by default preserves the current combatant, the original behavior
2. with the behavior I suggest, it would be possible to preserve the combatTurn (instead of the combatant) by setting turnEvents to false.

This would give modules simple and more explicit way of controlling the behavior compared to extending and overriding.

Now, the solution I have now works (and is more elegant when adding a custom operation "yzecSwapInitiative" and testing for it) but it took me quite some time to understand the code and figure out a solution that I think could have been easier in the first place.

Lastly, my suggestion might have implications that I have not considered so ofc I respect any decision you make with regards to this :)

Edit: I would also like to add that this is how it worked in v11 and part of v12 (after my previous bug report, see link above).

[aaclayton]

Whether or not the combatTurn changes needs to be unrelated to the value of turnEvents which controls whether downstream events for turn changes are emitted.

You can make a combatant update that explicitly keeps the turn the same by assigning that option to the operation, i.e.

combat.updateEmbeddedDocuments("Combatant", updates, {combatTurn: combat.turn});

I think this is the correct way to do a swap while keeping the numeric turn stable. Can you verify this approach resolves (or not) your neeed? Tentatively closing as wontfix.

[pafvel]

Thanks, verified working! It was obvious when I saw your comment. I was so close, but stopped one logical step from the right answer... :D Thanks for straightening things out!