Separate the reading and interpreting of a replay file #142
Conversation
|
I did a hiccup, all the changes are in a8889f8 . I also tried to use LFS and that turned it into an even bigger mess when trying to remove it again. |
|
I'll fix the pull request tomorrow. |
Sheikah45
force-pushed
the
rework/separate-file-reading
branch
from
0608f8b to
01f0f6e
Compare
There was a problem hiding this comment.
I don't know if there is a strong case for having the FromMemory public api methods. I don't really see a time when the replay will already be in memory rather than read directly from a file. Making these not public also removes the need for the ReplayBinaryFormat Wrapper classes as then the search for the separation between the metadata and command bytes is just an implementation detail.
There was a problem hiding this comment.
Taken this into account - in future pull requests the wrapper will go again but at this stage it helps me understand the code as I rework it 😄
|
The concept is not complete yet, but in the current state it is ready for a merge in my opinion. It doesn't impact existing functionality and it's covered by a few tests. The end goal is to use the newly created Eventually the
The Then, from that point onwards we can apply semantics to the parsed replay. This is now still combined in one class: the replay data parser. We can keep that class backwards compatible with the intended direction. |
| import java.util.Arrays; | ||
| import java.util.Objects; | ||
|
|
||
| public class Replay { |
There was a problem hiding this comment.
I think calling this ReplayLoader makes it clearer what this class is
| public sealed interface ReplayBinaryFormat { | ||
|
|
||
| record WithContext (byte[] bytes) implements ReplayBinaryFormat {} | ||
|
|
||
| record BinarySCFA(byte[] bytes) implements ReplayBinaryFormat {} | ||
|
|
||
| } |
There was a problem hiding this comment.
I don't think these classes are necessary anymore. As it is know they are only used as a wrapper around the bytes and only a member of the ReplayContainer. But they are only created and used inside the ReplayLoader without being exposed publicly. So I think the ReplayLoader can just handle the bytes appropriately without these types.
|
Closed in favor of #143 |
* Separate out the reading and interpreting of the file * Undo lfs of fafreplay * Fix replays that were damaged by LFS * Make the memory-based functions private * Clean up of the code * Improve documentation of unknown values * Document the header of a replay * First working end-to-end example of the new replay parsing * Restructure the project, process feedback by Sheikah in #142 * Extend tests and fix a few bugs * Discover the byte that is set when queueing orders * Introduce the first semantics * Add interpretation of chat messages * Introduce additional semantics * Use atomics * Rework the enums related to game options * Remove excessive whitespace * Add documentation that it still requires to be implemented * Fix typo in name * Rename 'Utils' to 'LoadUtils' * Use a better describing exception * Undo formatting changes * Remove the tokenizer of the header * Extend pattern matching of modern Java * DO THAT SCREAMING SNAKE CASE THING * Process feedback --------- Co-authored-by: Sheikah45 <[email protected]>
Factors out the reading of the file on disk. This is now a separate class. I intentionally did not touch the
ReplayDataParserclass because that would only complicate the pull request.The next pull request will separate the header from the body of a replay. The header is not to be confused with the additional JSON context that FAF introduces, that's a different concept. We'll extend the replay container in the process.
Related pull requests: