Rework of parsing the header of a replay #143

Garanas · 2024-05-07T09:46:06Z

In this pull request we finalize the new setup to process replays. As described in #140 , this is the final step towards separating the various phases of the parsing. With these changes we separate the following phases for the body of the replay:

(1) Token phase
(2) Parsing phase
(3) Semantics (interpretation) phase

New pull requests can now create additional semantics without being troubled by the token and parsing phases.

faf-commons-data/src/test/java/com/faforever/commons/replay/LoadReplayLoaderTest.java

Garanas · 2024-05-07T19:04:13Z

@Sheikah45 How would I verify various properties such as the token length being equal to the event length when those properties are not exposed?

Sheikah45 · 2024-05-07T19:29:53Z

I would ask if the bytes aren't exposed why do they need to be verified by a test at all?

Wouldn't it be equivalent to just check the events directly?

And in a test you know the tokens should be equal to the events so you just need to assert the number of events is as expected for a replay file.

Also as a side note, I don't think the records that wrap a single class are necessary, unless you have plans to extend them later on but from what I can see that doesn't seem to be the case.

Garanas · 2024-05-07T19:36:28Z

Also as a side note, I don't think the records that wrap a single class are necessary, unless you have plans to extend them later on but from what I can see that doesn't seem to be the case.

Are you referring to a record such as GameOption? What would you replace them with?

Sheikah45 · 2024-05-07T19:53:55Z

I meant ReplayBody sorry for not clarifying

Sheikah45 · 2024-05-07T19:57:35Z

Also as a side note we don't generally use annotations such as @ Contract since the upkeep of those isn't necessarily worth the benefit from the ide analysis for our repos

faf-commons-data/src/main/java/com/faforever/commons/replay/shared/LuaTable.java

Garanas · 2024-05-08T04:37:16Z

I meant ReplayBody sorry for not clarifying

I understand, it's primarily to make it conceptually clear. I'll remove it again

Garanas · 2024-05-08T04:38:40Z

I would ask if the bytes aren't exposed why do they need to be verified by a test at all?

To verify everything is correct from the bottom up. Instead, I've introduced conditions that throw an exception when things are off during parsing. That also ironically forced me to fix two bugs, so that's nice 😃

Garanas · 2024-05-08T04:40:20Z

I'd like to add some more replays (5 from co-op, 5 from faf, 5 from fafdevelop, etc) to more solidify the current setup. I'll also test other properties such as the number of events of a given type, to catch regression if more changes are made in the future.

Would that test the feature sufficient?

Sheikah45 · 2024-05-08T23:57:08Z

Why do we need 5 of each replay?

That seems like a bit much given they don't contain very different structures

Sheikah45 · 2024-05-08T23:59:50Z

I would normally think even just one test for parsing is sufficient given it is not a stochastic process and they don't contain unknown events

Sheikah45 · 2024-05-09T00:01:07Z

faf-commons-data/src/main/java/com/faforever/commons/replay/ReplayDataParser.java

+    int sizeGameOptionsInBytes = dataStream.readInt();
+    this.gameOptions = ((Map<String, Object>) parseLua(dataStream)).entrySet().stream().filter(entry -> "Options".equals(entry.getKey())).flatMap(entry -> ((Map<String, Object>) entry.getValue()).entrySet().stream()).map(entry -> new GameOption(entry.getKey(), entry.getValue())).collect(Collectors.toList());


leave this wrapped as it is more readable

That's the formatter!! 😛

Processed in:

49ddff9

Sheikah45 · 2024-05-09T00:09:21Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/GameOptions.java

+  public enum AutoTeams {
+    none("None"),
+    manual("Manual"),
+    tvsb("Top versus bottom"),
+    lvsr("Left versus right"),
+    pvsi("Even versus uneven");
+
+    private final String string;
+
+    AutoTeams(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum TeamLock {
+    locked("Locked"), unlocked("Unlocked");
+
+    private final String string;
+
+    TeamLock(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum TeamSpawn {
+    fixed("Fixed"),
+    random("Random"),
+    balanced("Balanced"),
+    balanced_flex("Flexible balanced"),
+    random_reveal("Random and revealed"),
+    balanced_reveal("Balanced and revealed"),
+    balanced_reveal_mirrored("Mirror balanced and revealed"),
+    balanced_flex_reveal("Flexible balanced and revealed");
+
+    private final String string;
+
+    TeamSpawn(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum Victory {
+    demoralization("Assasination"),
+    domination("Supremacy"),
+    eradication("Annihilation"),
+    sandbox("Sandbox");
+
+    private final String string;
+
+    Victory(String string) {
+      this.string = string;
+    }
+  }


Java enums should be SCREAMING_SNAKE_CASE and AutoTeams can be the full name for the enum

Also the strings don't have getters so aren't accessible outside the enum

How would I then match the game options with the enums? At the moment they are demoralization, domination, etc which I think I can immediate map one-to-one to an enum.

You would add a findByValue method that takes in a string and returns the enum that matches the string you define as a member of the enum.

Noting that the enum names should be normalized

Processed in cce2ee3

Garanas · 2024-05-09T06:32:40Z

I would normally think even just one test for parsing is sufficient given it is not a stochastic process and they don't contain unknown events

Not all replays have all information in them however. I think I've covered almost all of the cases at this moment

Garanas · 2024-05-09T06:33:54Z

With d0e9566 we introduce the first semantics: applying the tick and the client to an event! It also casually shrinks the number of events of a large replay from ~130k to ~15k, which makes it much more feasible to process too 😃

Sheikah45 · 2024-05-09T23:27:05Z

faf-commons-data/src/main/java/com/faforever/commons/replay/semantics/Semantics.java

+    final int[] tick = {-1};
+    final int[] clientId = {-1};


I would just use an AtomicInteger here

Processed in 0f0aca2

Sheikah45 · 2024-05-09T23:35:33Z

faf-commons-data/src/main/java/com/faforever/commons/replay/semantics/Semantics.java

+      case Event.LuaSimCallback(
+        String func, LuaTable parametersLua, Event.CommandUnits commandUnits
+      ) when func.equals("GiveResourcesToPlayer") -> {
+        yield switch (parametersLua) {
+          case LuaTable.Table callbackTable -> {


You don't need the internal switch. If you type parametersLua as LuaTable.Table the case will only match when that is the case

Suggested change

case Event.LuaSimCallback(

String func, LuaTable parametersLua, Event.CommandUnits commandUnits

) when func.equals("GiveResourcesToPlayer") -> {

yield switch (parametersLua) {

case LuaTable.Table callbackTable -> {

case Event.LuaSimCallback(

String func, LuaTable.Table callbackTable, Event.CommandUnits commandUnits

) when func.equals("GiveResourcesToPlayer") -> {

And since this is only one case label we can just use an if (!( instanceof))

I removed the internal switch in 7d4ef66

I don't know how to rewrite the instance check and also keep the when statement, or are those two separate if statements?

You would just change it from when to a normal && condition

I'd rather keep the current format. It makes it consistent with the other semantic-related functions. If you want me to change it anyway then I'll do that; but I think consistency is nice.

Garanas · 2024-05-11T07:57:30Z

This pull request is ready for feedback - functionally it is where I'd want it to be and further semantics can be introduced as separate pull requests.

I'll only focus on extending the test suite and processing the feedback.

Sheikah45 · 2024-05-11T08:47:23Z

faf-commons-data/src/main/java/com/faforever/commons/replay/body/ReplayBodyParser.java

+      if(stream.available() > 0) {
+        throw new EOFException();
+      }


This a little confusing use of exception. typically EOFException means we hit the end of the file when we are expecting more data. Here we throw an exception if there is data left over.

I think probably just an illegal state exception with appropriate message is best

I agree it is semantically better, I was searching for a proper exception when writing the code. Processed it in:

4348c64

Sheikah45 · 2024-05-11T08:48:25Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/GameOptions.java

+  public enum AutoTeams {
+    none("None"),
+    manual("Manual"),
+    tvsb("Top versus bottom"),
+    lvsr("Left versus right"),
+    pvsi("Even versus uneven");
+
+    private final String string;
+
+    AutoTeams(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum TeamLock {
+    locked("Locked"), unlocked("Unlocked");
+
+    private final String string;
+
+    TeamLock(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum TeamSpawn {
+    fixed("Fixed"),
+    random("Random"),
+    balanced("Balanced"),
+    balanced_flex("Flexible balanced"),
+    random_reveal("Random and revealed"),
+    balanced_reveal("Balanced and revealed"),
+    balanced_reveal_mirrored("Mirror balanced and revealed"),
+    balanced_flex_reveal("Flexible balanced and revealed");
+
+    private final String string;
+
+    TeamSpawn(String string) {
+      this.string = string;
+    }
+  }
+
+  public enum Victory {
+    demoralization("Assasination"),
+    domination("Supremacy"),
+    eradication("Annihilation"),
+    sandbox("Sandbox");
+
+    private final String string;
+
+    Victory(String string) {
+      this.string = string;
+    }
+  }


Noting that the enum names should be normalized

Sheikah45 · 2024-05-11T08:50:19Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/ReplayHeaderParser.java

+
+
+
+
+
+      return null;


Why all the whitespace? And why return null instead of an empty list?

This is not implemented yet. I intend to implement it in a future pull request. I did remove the whitespace and document it in comments:

a18552a

2caafd3

Sheikah45 · 2024-05-11T08:50:40Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/ReplayHeaderParser.java

+
+
+
+


Same whitespace question

See the previous response

Sheikah45 · 2024-05-11T08:50:54Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/ReplayHeaderParser.java

+
+
+
+


Same whitespace question

See the previous response

Sheikah45 · 2024-05-11T08:52:56Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/ReplayHeaderToken.java

+public record ReplayHeaderToken(String gameVersion, String replayVersion, String pathToScenario, boolean cheatsEnabled, int seed,
+                                List<Source> sources,
+                                byte[] mods,
+                                byte[] gameOptions,
+                                List<byte[]> playerOptions) {
+}


Why did you decide to have the header be represented by this more raw value and a more complete value in ReplayHeader? I don't know if there is the same value to having both of these that there is for the body tokens.

I was a little bit at a loss here. The header is not as nicely structured as the body of a replay. I did find a few cases where the header has a leading 'size' integer and I interpret those as 'I'll process whatever is in there during parsing'

I think it is probably better to just parse all the fields directly rather than having two objects since there isn't exactly the same issue with new tokens and the size notation.

That would forfeit the separation of phases; should I then just process/parse everything in the ReplayHeaderParser class?

Yeah I would think so. Since it seems like the header is really more like a single set of bytes.

Processed in 76615a8

Sheikah45 · 2024-05-11T08:54:16Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/Source.java

+ * @param name
+ * @param sourceId
+ */
+public record Source(int sourceId, int PlayerId, String name) {


playerId with proper casing

Processed in:

07bee25

Sheikah45 · 2024-05-11T08:55:33Z

faf-commons-data/src/main/java/com/faforever/commons/replay/shared/Utils.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class Utils {


I would probably rename to LoadUtils or ParseUtils

Processed in 020955d

Sheikah45 · 2024-05-11T08:57:57Z

faf-commons-data/src/main/java/com/faforever/commons/replay/ReplaySemantics.java

+    final AtomicInteger tick = new AtomicInteger(0);
+    final AtomicInteger clientId = new AtomicInteger(-1);
+
+    return events.stream().map((event) -> switch (event) {
+      case Event.Advance e -> {
+        tick.addAndGet(e.ticksToAdvance());
+        yield null;
+      }
+
+      case Event.SetCommandSource e -> {
+        clientId.set(e.playerIndex());
+        yield null;
+      }
+
+      default -> new RegisteredEvent(tick.intValue(), sources.get(clientId.intValue()), event);
+    }).filter(Objects::nonNull).toList();


Looking at this now I might just change this to a normal enhanced for loop that modifies a list since the lambda body modifies the external state so much. Then just a normal int can be used. But I don't know if it is totally required.

I think it is nice to be consistent with other solutions that we use to map over the events.

Sheikah45 · 2024-05-11T10:01:34Z

@Brutus5000 don't want to leave you out this time XD

Sheikah45 · 2024-05-12T13:00:31Z

faf-commons-data/src/main/java/com/faforever/commons/replay/ReplaySemantics.java

+      case Event.Advance e -> {
+        tick.addAndGet(e.ticksToAdvance());
+        yield null;
+      }
+
+      case Event.SetCommandSource e -> {
+        clientId.set(e.playerIndex());
+        yield null;
+      }


You can use the full pattern matching form here so Event.Advance(int ticksToAdvance)

Processed in 706ad77

Sheikah45 · 2024-05-12T13:03:50Z

faf-commons-data/src/main/java/com/faforever/commons/replay/header/GameOptions.java

+    fixed("fixed", "Fixed"),
+    random("random", "Random"),
+    balanced("balanced", "Balanced"),
+    balanced_flex("balanced_flex", "Flexible balanced"),
+    random_reveal("random_reveal", "Random and revealed"),
+    balanced_reveal("balanced_reveal", "Balanced and revealed"),
+    balanced_reveal_mirrored("balanced_reveal_mirrored", "Mirror balanced and revealed"),
+    balanced_flex_reveal("balanced_flex_reveal", "Flexible balanced and revealed");


These enums should be properly cased as well

Processed in: bdee610

Sheikah45

One comment that could or could not be done really

Sheikah45 · 2024-05-17T19:21:07Z

faf-commons-data/src/main/java/com/faforever/commons/replay/ReplaySemantics.java

+        if (!(callbackTable.value().get("From") instanceof LuaData.Number from)) {
+          yield null;
+        }
+
+        // focus army starts is 1-based instead of 0-based, to align it we subtract 1
+        if (from.value() - 1 <= -2) {
+          yield null;
+        }
+
+        // TODO: this field has no meaning and can be manipulated, instead use the authorised command source.
+        //  Requires refactoring in the game!
+        if (!(callbackTable.value().get("Sender") instanceof LuaData.String sender)) {
+          yield null;
+        }
+
+        // TODO: apparently all players create a sim callback that contains the chat message. This hack is how we skip it,
+        //   Requires refactoring in the game!
+        if (!Objects.equals(sender.value(), registeredEvent.source().name())) {
+          yield null;
+        }
+
+        if (!(callbackTable.value().get("Msg") instanceof LuaData.Table msgTable)) {
+          yield null;
+        }
+
+        // TODO: this is 1 out of the 2 legitimate fields
+        if (!(msgTable.value().get("to") instanceof LuaData.String msgTo)) {
+          yield null;
+        }
+
+        // TODO: this is 2 out of the 2 legitimate fields
+        if (!(msgTable.value().get("text") instanceof LuaData.String msgText)) {
+          yield null;
+        }


Technically for these you could use the full record pattern. So LuaData.Number(float value)

Just to remove all the calls of .value

Processed in 1b9492f

Garanas and others added 7 commits May 6, 2024 17:01

Separate out the reading and interpreting of the file

1403423

Undo lfs of fafreplay

01f0f6e

Fix replays that were damaged by LFS

01f3bc2

Make the memory-based functions private

20155f0

Clean up of the code

a5ee027

Improve documentation of unknown values

c7f75b4

Document the header of a replay

bccff78

Garanas added enhancement feature: replays labels May 7, 2024

Garanas changed the title ~~Rework parsing the header of a replay~~ Rework of parsing the header of a replay May 7, 2024

Garanas added 2 commits May 7, 2024 20:51

First working end-to-end example of the new replay parsing

a8e7303

Restructure the project, process feedback by Sheikah in #142

43c0572

Garanas mentioned this pull request May 7, 2024

Separate the reading and interpreting of a replay file #142

Closed

Garanas commented May 7, 2024

View reviewed changes

faf-commons-data/src/test/java/com/faforever/commons/replay/LoadReplayLoaderTest.java Outdated Show resolved Hide resolved

Extend tests and fix a few bugs

8558a85

Sheikah45 reviewed May 7, 2024

View reviewed changes

faf-commons-data/src/main/java/com/faforever/commons/replay/shared/LuaTable.java Outdated Show resolved Hide resolved

Discover the byte that is set when queueing orders

a5467f0

Sheikah45 reviewed May 9, 2024

View reviewed changes

Introduce the first semantics

d0e9566

Add interpretation of chat messages

dbb816b

Sheikah45 reviewed May 9, 2024

View reviewed changes

Garanas added 2 commits May 11, 2024 09:37

Introduce additional semantics

7d4ef66

Use atomics

0f0aca2

Garanas marked this pull request as ready for review May 11, 2024 07:53

Garanas requested review from Sheikah45 and Brutus5000 May 11, 2024 07:56

Sheikah45 requested changes May 11, 2024

View reviewed changes

Garanas added 7 commits May 11, 2024 11:35

Rework the enums related to game options

cce2ee3

Remove excessive whitespace

a18552a

Add documentation that it still requires to be implemented

2caafd3

Fix typo in name

07bee25

Rename 'Utils' to 'LoadUtils'

020955d

Use a better describing exception

4348c64

Undo formatting changes

49ddff9

Sheikah45 reviewed May 12, 2024

View reviewed changes

Garanas added 3 commits May 17, 2024 13:15

Remove the tokenizer of the header

76615a8

Extend pattern matching of modern Java

706ad77

DO THAT SCREAMING SNAKE CASE THING

bdee610

Garanas requested a review from Sheikah45 May 17, 2024 11:20

Sheikah45 reviewed May 17, 2024

View reviewed changes

Process feedback

1b9492f

Sheikah45 approved these changes May 18, 2024

View reviewed changes

Sheikah45 merged commit 6e9ba4d into develop May 18, 2024
1 check passed

Sheikah45 deleted the rework/replay-header branch May 18, 2024 14:06

		int sizeGameOptionsInBytes = dataStream.readInt();
		this.gameOptions = ((Map<String, Object>) parseLua(dataStream)).entrySet().stream().filter(entry -> "Options".equals(entry.getKey())).flatMap(entry -> ((Map<String, Object>) entry.getValue()).entrySet().stream()).map(entry -> new GameOption(entry.getKey(), entry.getValue())).collect(Collectors.toList());

Rework of parsing the header of a replay #143

Rework of parsing the header of a replay #143

Conversation

Garanas commented May 7, 2024 • edited Loading

Garanas commented May 7, 2024

Sheikah45 commented May 7, 2024 • edited Loading

Garanas commented May 7, 2024

Sheikah45 commented May 7, 2024

Sheikah45 commented May 7, 2024

Garanas commented May 8, 2024

Garanas commented May 8, 2024 • edited Loading

Garanas commented May 8, 2024

Sheikah45 commented May 8, 2024

Sheikah45 commented May 8, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garanas commented May 9, 2024

Garanas commented May 9, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garanas commented May 11, 2024

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

Garanas May 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garanas May 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sheikah45 commented May 11, 2024

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

Sheikah45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garanas commented May 7, 2024 •

edited

Loading

Sheikah45 commented May 7, 2024 •

edited

Loading

Garanas commented May 8, 2024 •

edited

Loading

Garanas May 11, 2024 •

edited

Loading

Garanas May 11, 2024 •

edited

Loading