Save the owner of the new units in case another action changes the ownership #6943

trevan · 2020-06-30T05:43:34Z

In Warcraft War Heroes, units are transformed into "loot" that is then captured by the winner. When the loot is first added to the battle, it is owned by the loser. This causes a series of change commands:

Remove Unit
Add Unit
Change Unit's Owner

When the AI is playing and the user is spectating, the history also sees these change commands and converts them to its local gameData.

The problem occurs when the steps happen in the following manner:

Game - Add Unit
Game - Change Unit's Owner
History - Add Unit
Game - Change Unit's Owner

When 3 runs, it is using the unit that had its owner changed in 2. So the change owner in 4 throws an error because the owner is different from what it is expecting.

This fix stores the original owner when the unit is added. So when 3 runs, it can ensure that the unit has the original owner and not the new one.

Functional Changes

[] New map or map update
[] New Feature
[] Feature update or enhancement
[] Feature Removal
[] Code Cleanup or refactor
[] Configuration Change
[x] Problem fix
[] Other:

Testing

Loaded a save that had the exception and ensured that the exception didn't happen. Also checked that save games still loaded.

Screens Shots

Additional Notes to Reviewer

Release Note

FIX|PlayerOwnerChange exception in Warcraft War Heroes is fixed

…nership

DanVanAtta · 2020-06-30T05:55:22Z

Nice work digging into this @trevan 👍

I'll try to review this in detail tomorrow if @RoiEXLab does not get to it sooner.

…ge-error

trevan · 2020-06-30T16:20:47Z

The smoke test failed but I don't think this change caused that.

prastle · 2020-06-30T21:51:52Z

Wow! Welcome to the team! @trevan

DanVanAtta · 2020-07-01T20:37:18Z

Perhaps a retry on the smoke-test would be in order (in general), it failed here due to a transient download problem:

Downloading https://services.gradle.org/distributions/gradle-6.5-all.zip
..
Exception in thread "main" javax.net.ssl.SSLProtocolException: Connection reset

DanVanAtta · 2020-07-01T20:39:08Z

game-core/src/main/java/games/strategy/engine/data/changefactory/AddUnits.java

    this.type = type;
    this.name = name;
  }

+  private Map<UUID, String> getUnitPlayerMap(final Collection<Unit> units) {


nit: 'get' methods usually are just 'getters'. If computation is done a different verb could be easier for future maintainers. Perhaps rewording this to buildUnitPlayerMap would help that. WDYT @trevan ?

This could be a moot comment if it turns out we do not need a map.

DanVanAtta · 2020-07-01T20:42:19Z

game-core/src/main/java/games/strategy/engine/data/changefactory/AddUnits.java

+  private Collection<Unit> getUnitsWithOwner(final GameData data) {
+    final Map<UUID, Unit> uuidToUnits =
+        units.stream().collect(Collectors.toMap(Unit::getId, unit -> unit));
+    return unitPlayerMap.entrySet().stream()


If we're iterating over the entry set of the unitPlayerMap, why do we need a map for this? Can we not just iterate over the set of units and avoid the map data structure altogether?

The original code just iterated over the set of units. That's the problem. We need to store the map of unit -> owner outside of the units so that when the owner changes in the unit, it doesn't change the map here.

Cool, I think that probably should be called out to make it clear & obvious. Perhaps a javadoc comment on the map property would do the trick. WDYT?

I added some documentation and changed the method names. WDYT?

DanVanAtta · 2020-07-01T21:57:19Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- game-core/src/main/java/games/strategy/engine/data/changefactory/AddUnits.java  1

See the complete overview on Codacy

DanVanAtta

👍

DanVanAtta · 2020-07-01T21:57:12Z

game-core/src/main/java/games/strategy/engine/data/changefactory/AddUnits.java

    this.type = type;
    this.name = name;
  }

+  private Map<UUID, String> buildUnitOwnerMap(final Collection<Unit> units) {


side-note / big nit, this method could be static. I think we need to try and see if we can share a config that turns on the static analysis warning that methods can be marked as static.

For me it's really helpful as I watch for private methods being static. If they are, I know they do not interact with the class state. If they are not static, I assume they cannot be static and hence interact with class state.

DanVanAtta · 2020-07-01T21:58:22Z

game-core/src/main/java/games/strategy/engine/data/changefactory/AddUnits.java

@@ -36,11 +53,36 @@ public Change invert() {
  @Override
  protected void perform(final GameData data) {
    final UnitHolder holder = data.getUnitHolder(name, type);
-    holder.getUnitCollection().addAll(units);
+    final Collection<Unit> unitsWithCorrectOwner = buildUnitsWithOwner(data);
+    holder.getUnitCollection().addAll(unitsWithCorrectOwner);


A test of some sort to verify the right functionality would be a good thing to have. If we change & break this, we'll have the extra safety net. Testing is really good for ensuring correctness, which is a further safety net that would be good to have as well.

DanVanAtta · 2020-07-01T21:59:24Z

Waiting for build to complete, good to merge once that finishes.

trevan · 2020-07-01T22:11:15Z

I kind of feel like this is more of a band-aid vs a true fix. I bet I could cause this same type of problem by adding a new unit and then changing its hitpoints or any other edit to the unit.

I think the long term fix is to clone the unit when it is added to this change (and other changes). That way, the original unit can be changed by later changes but the unit in this change object will stay the way it was initially. Is it it ok to try and implement Cloneable on Unit?

DanVanAtta · 2020-07-01T22:13:21Z

Would an immutable copy of the unit state work instead of clone? AFAIK clone has its pit-falls, generally a bit error-prone ot use.

DanVanAtta · 2020-07-01T22:14:45Z

IMO changes should generally contain immutable data. If we maintained a stack of change data, it could be a really good avenue for having better save games. We could serialize the change stack and replay them on a base game data instead of serializing a game data. That kind of change I think would be a big breakthrough for compatibility.

trevan · 2020-07-01T22:16:14Z

That should work as well, though I'm not entirely sure how to do that in a non-clone manner. Is there something like that in the code base already?

DanVanAtta · 2020-07-01T23:15:08Z

Creating a copy-constructor would do it.
Keeping the original data immutable is good.
Eventually we won't be wiring a gameData reference to everything (which we only do for save-game), that'll make it easier for the data to be immutable.
We can use a state tracking inner object, so you only need to copy that state (similar to serialization proxy pattern)
'lombok' 'toBuilder' is a decent way to get a copy-constructor for free: https://projectlombok.org/features/Builder

Overall I think the band-aid is probably fine for now until we redo how game serialization is done.

Save the owner of the new units in case another action changes the ow…

a669461

…nership

trevan added 2 commits June 30, 2020 08:39

Handle case where unit isn't in the gameData yet

db68d72

Merge remote-tracking branch 'upstream/master' into player-owner-chan…

59b252e

…ge-error

DanVanAtta reviewed Jul 1, 2020

View reviewed changes

Improve naming and add documentation for the unit-owner map

9d9e114

DanVanAtta approved these changes Jul 1, 2020

View reviewed changes

DanVanAtta merged commit 9402b78 into triplea-game:master Jul 1, 2020

trevan deleted the player-owner-change-error branch July 2, 2020 04:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save the owner of the new units in case another action changes the ownership #6943

Save the owner of the new units in case another action changes the ownership #6943

trevan commented Jun 30, 2020

DanVanAtta commented Jun 30, 2020

trevan commented Jun 30, 2020

prastle commented Jun 30, 2020

DanVanAtta commented Jul 1, 2020

DanVanAtta Jul 1, 2020

DanVanAtta Jul 1, 2020

DanVanAtta Jul 1, 2020

trevan Jul 1, 2020

DanVanAtta Jul 1, 2020

trevan Jul 1, 2020

DanVanAtta commented Jul 1, 2020

DanVanAtta left a comment

DanVanAtta Jul 1, 2020

DanVanAtta Jul 1, 2020

DanVanAtta commented Jul 1, 2020

trevan commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020 •

edited

Loading

trevan commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020

Save the owner of the new units in case another action changes the ownership #6943

Save the owner of the new units in case another action changes the ownership #6943

Conversation

trevan commented Jun 30, 2020

Functional Changes

Testing

Screens Shots

Additional Notes to Reviewer

Release Note

DanVanAtta commented Jun 30, 2020

trevan commented Jun 30, 2020

prastle commented Jun 30, 2020

DanVanAtta commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta commented Jul 1, 2020

DanVanAtta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta commented Jul 1, 2020

trevan commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020 • edited Loading

trevan commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020

DanVanAtta commented Jul 1, 2020 •

edited

Loading