Preparations for RPG_RT - Player regression testing

[Ghabry]

With PRs such as #2208 where normal code reviews fail we finally need an automatic regression tester.

@fmatthew5876 I could need your help here. I ask you to provide two things. It would help me when you can contribute them (best would be as a seperate PR, so I can use it in both master and your character PR)

My idea is the following:

1. Add a "unit test" mode to Player which saves the game each frame
2. Replay the input in RPG_RT through injected DLL while saving the game each frame (for synchronization the Game_System.frame_counter is used)
3. Compare the save files
4. .....
5. PROFIT

---

TODO

• [x] Invent a better input recording format [1] @Ghabry
• [x] Add a new SavegameManager class that provides a Save(int save_id) function. @fmatthew5876 can you help with this? A Load function isn't needed yet but I need a globally accesible Save. [2]
• [x] Write a unit test mode that saves the game each frame (easy to add when Save() exists)
• [x] Write a DynRpg plugin that can replay input and save each frame @Ghabry
• [ ] Write an automatic difftool (python based?), could parse lcf2xml output and skip chunks that usually don't match (see [2])
• [ ] Record some tests

Known issues

• [x] DynRPG only writes up to Save99.lsd, needs some trickery (file renaming) to record longer
• [x] The recording desyncs when there is a teleport. Is off by 5 frames?!

[1] My suggestion: https://github.com/EasyRPG/Player/commit/6531fd8cc4c08d1b53af0a33c7e343dea5e2da54 The important lines are the "F" lines, the format is game_system_frame,keys..... A frame can occur multiple times because Game_System frames are reset in some cases, but this is no problem.

H EasyRPG Player Recording
V 2
D 2020-08-23-23-27-37
F 45,DECISION
F 0,DECISION
F 5,RIGHT
F 6,RIGHT
[...] <- A line is only written when there was an input in this frame
F 28,RIGHT,UP
F 29,RIGHT,UP
F 30,RIGHT,UP

[2]

Notes about savegame incompatibility

• title.timestamp - this varies (obviously)
• system.scene is 0 on the map
• easyrpg.version - only we write this

Strange observations

• system.save_count when using dynrpg save this is always 0
• system.save_slot when using dynrpg save this is always 1

Incompatible save storage

• Player stores <saveactor><status>0 0 0 0 0 0 0 0 0 0</status>, but RPG_RT stores <status></status> @fmatthew5876 can you fix this? I guess RPG_RT only writes numbers until it reaches all zero.

Lower priority

• [ ] Write a program that injects a DLL in RPG_RT that is DynRPG independent and provides the required features

[Ghabry]

Further idea via IRC:

The savegame could be a concatenated file.

Maybe add a --unittest <NAME> flag. When set the Player appends a save each frame to a <NAME> file. NOTE: Don't write while on the title screen. @fmatthew5876

[fmatthew5876]

I looked into the states thing, unfortunately it's a bit tricky. RPG_RT actually resizes the states array the first time an actor is added to your party, regardless of whether they have states or not.

Ultimately, I think this and other related things depends on https://github.com/EasyRPG/liblcf/issues/371

That is we should get rid of these setup and fixup functions. Liblcf should just read and present whatever in raw form, and then we do any fixing and adjusting in player.

It's on my todo list. But want to get the string/array optimization stuff in first to avoid rebase problems.

[fmatthew5876]

Add a new SavegameManager class that provides a Save(int save_id) function. @fmatthew5876 can you help with this? A Load function isn't needed yet but I need a globally accesible Save. [2]

Ultimately my objective for this one is part of https://github.com/EasyRPG/Player/issues/1954

That is if we create a new Game_Data singleton which is responsible for initializing, storing, and tearing down all the game state, it can have a Save(slot) function which saves it all.

The problem of course is this has a dependency on #2208, but we want to have this now so we can test #2208 !

Therefore as a temporary solution I can just rip the saving functionality out of Scene_Save and make it globally accessible so we're not blocked.

Notes about savegame incompatibility

We're going to need some kind of "black list" config file which contains the set of chunks to ignore. This would either be used to omit chunks from the saves (since they not intended to be loaded, this is fine), or later in the pipeline as part of the diffing tool. I would do it wherever it is easiest and most convenient as a first step.

Write an automatic difftool (python based?), could parse lcf2xml output and skip chunks that usually don't match (see [2])

For a long replay this could be slow. Maybe we can binary diff the save files first, and only if they don't match, generate xml and diff again to show it in a human readable fashion?

Anyway, I would put such optimization concerns off until after the first version.

[Ghabry]

Therefore as a temporary solution I can just rip the saving functionality out of Scene_Save and make it globally accessible so we're not blocked.

Okay but please also provide a Filesystem_Inputstream version for handling the "append mode".

or later in the pipeline as part of the diffing tool. I would do it wherever it is easiest and most convenient as a first step.

Diffing tool is easier for now. Performance doesn't matter yet for the prototype.

For a long replay this could be slow.

Yeah for match the hash is fine but we are not there yet.

[Ghabry]

save_count and save_slot is handled by the File scene in RPG_RT. This explains the wrong values.

GetLocalTime can be hooked to modify title.timestamp.

[Ghabry]

The DynRPG plugin is finished https://github.com/EasyRPG/regression-tests/tree/master/tools/regtest-plugin

Usage:

wine rpg_rt.exe TestPlay X Window --replay-input input.log --regression-test save-concat.lsd

[Ghabry]

Here a python script for extracting all saves from a concatenated save. Unfortunately there is no size field after the header so I have to process all top chunks (advantage: Better detection of corrupted files)

This was the difficult part of the script imo. Missing is only: Read two of these files, then hash compare LSD by LSD and when they mismatch extract them, convert them with lcf2xml and output a diff.

import sys

chunks = list(range(0x64, 0x72+1))

save_data = []
cur_save_data = bytearray(b"")

def read(stream, count):
    b = stream.read(count)
    cur_save_data.extend(b)
    return b

def read_ber(stream):
    value = 0;
    temp = 0x80;
    while temp & 0x80:
        value <<= 7
        temp = int.from_bytes(read(stream, 1), byteorder='little')
        value |= (temp & 0x7F)
    return value

def read_save(stream):
    saves = len(save_data) + 1

    if read(f, 11).decode("ascii") != "LcfSaveData":
        raise ValueError(f"Save {saves} corrupted: Expected LcfSaveData")

    while (True):
        chunk = read_ber(f)
        if chunk not in chunks:
            if chunk == 0xB:
                # Start of new save
                del cur_save_data[-1]
                return True
            elif chunk == 200:
                # Likely not what you want for a compare
                print(f"Warning: Save {saves} contains EasyRPG chunk")
            elif chunk == 0:
                if not stream.read(1):
                    # EOF
                    return False
            raise ValueError(f"Save {saves} corrupted: Bad chunk {chunk}")
        chunk_size = read_ber(f)
        read(f, chunk_size)

with open(sys.argv[1], "rb") as f:
    if read(f, 1) != b"\x0b":
        raise ValueError(f"Not a save file")
    while (read_save(f)):
        save_data.append(cur_save_data)
        cur_save_data = bytearray(b"\x0b")
    save_data.append(cur_save_data)

print(f"Found {len(save_data)} savegames")

# Example: Extract them
for i, save in enumerate(save_data):
    with open(f"Save{i+1}.lsd", "wb") as f:
        f.write(save)

[Ghabry]

A problem with the savegame hashing: The hash varies but lcf2xml says "they are identical" (so I can still work with them, but is suspicious and not suitable for mass-testing -> slow).

Here are the two files: same.zip

---

Worth investigating:

Another logic difference but not really testable because RNG related: RPG_RT keeps "encounter calling" on TRUE even when there are no encounters on the map. Player resets the flag. Maybe this means the entering an area while this is T will force an encounter when the area is touched? Needs testing.

[Ghabry]

There appears to be an input lag of one frame or my hook is bad. For consistency I must use a custom hook point. Currently in RPG_RT this is OnFrame but in Player it is right before IncFrame is called. In both engines the hook must be exactly at the same position.

---

RPG_RT:

1. Calls sub_48FE98 <- This calls AsyncKeyState
2. scene update loop
3. inc frame counter
4. update the inputs
5. switch branching

So when hooking GetAsyncKeyState the frame counter is one behind. Maybe inc the frame counter to increase accuracy?

[fmatthew5876]

Another logic difference but not really testable because RNG related: RPG_RT keeps "encounter calling" on TRUE even when there are no encounters on the map. Player resets the flag. Maybe this means the entering an area while this is T will force an encounter when the area is touched? Needs testing.

This was observed in #2208 ? If so it's an incompatibility that should be fixed.

Looking at RPG_RT, it will always do the encounter rate calculations based on the map encounter rate, even if there are no monsters. encounter steps logic is only skipped if the encounter rate is 0.

Then when encounter_calling == 1 on the next frame, before checking for input, RPG_RT will always set the menu-calling and encounter_calling flags back to 0, and then setup a battle only if the map has enemies.

It looks like RPG_RT will only return and skip the rest of the player input logic if a battle started, and continue it if the battle is not started, which is different than what I do in #2208

I need to do some tests later on to verify this behavior.

[Ghabry]

yes I usually do these tests with #2208

I had a map with rate 25 and no enemies. The flag was true the entire (?) time.

[Ghabry]

More problems: Message boxes: The subcommand_path contains garbage data (this is known)... Maybe hook the Subcommand allocator function to get the Player initialisation (255)...

@fmatthew5876 message_active is never modified by Player.

Doesn't appear to be used for branching by RPG_RT. What is even the purpose of it. o_O

[fmatthew5876]

yes I usually do these tests with #2208

I had a map with rate 25 and no enemies. The flag was true the entire (?) time.

Are you absolutely sure about this? Every single frame?

I just tested RPG_RT with a debugger, while indeed I had a small deviation in #2208 which is now fixed, I am not at all seeing encounter_calling set every frame. I put a breakpoint on the place where it checks the flag and tries to setup the battle. This breakpoint was only triggered a few times while walking, as you would expect with the default encounter rate.

This was a 20x15 map, no enemies, 25 encounter rate.

Even with encounter rate of 1, I still only trigger a battle once per movement, not every frame.

The RPG_RT code does if (p > rand()) so by always returning 4, you're simulating always rolling true. Still I would expect it to happen once per movement, not once per frame.

The UpdateNextMovementAction() function which does all the encounter logic is only called when you're not already moving.

More problems: Message boxes: The subcommand_path contains garbage data (this is known)... Maybe hook the Subcommand allocator function to get the Player initialisation (255)...

The function is at 004ABE80

The is basically:

if (new_size > array.size) {
  array.size = new size;
  array.ptr = ReallocMem(array.ptr, new_size);
}

I'm not sure if this function is unique to subcommand_path or used elsewhere for other arrays.

According to this, pascal ReallocMem resizes the buffer but does not initialize the new data if it grows.

https://www.freepascal.org/docs-html/rtl/system/reallocmem.html

@fmatthew5876 message_active is never modified by Player.

Doesn't appear to be used for branching by RPG_RT. What is even the purpose of it. o_O

I looked at this a bit here: https://github.com/EasyRPG/Player/issues/1849

I haven't been able to figure this one out. As far as I can tell it's some half implemented feature that gets written to in some cases but has no real effect on anything. But I could be wrong

[Ghabry]

Are you absolutely sure about this? Every single frame?

Have to recheck this with my better hook. Maybe it was just true during the on frame callback and reset later...

[fmatthew5876]

Are you absolutely sure about this? Every single frame?

Have to recheck this with my better hook. Maybe it was just true during the on frame callback and reset later...

Even with that, UpdateEncounterSteps() is only called once when you start moving and creating the battle is called once when the movement finishes.

Unless something else is playing with these flags somehow.

Btw, I checked the subcommand path stuff, I'm pretty certain that function is only used by the interpreter, so it should be possible to hook it and initialize the enlarged portion of the array with 0xFF.

[Ghabry]

Another comment here that actually belongs in the PR but the PR history is hard to follow because Github collapses everything due to too many commits...

I didn't notice that there are multiple IncFrame calls. In Player I put it after IncFrame in Player and in RPG_RT I put it after ++frameCounter in the uhm RPG_SceneUpdateMain function (the ones which branches based on the scene ID, don't have the files on this PC - can't remember the name)

I don't think that I catch PreUpdate stuff.

[fmatthew5876]

With my last commit, you should be fine to follow the 2 frame increment hook points.

However it might be the case that input capture is done at different times. I think we do it before in Player and RPG_RT does it inside the scene update calls?

PreUpdate for new game happens in SceneTitle::Update() in RPG_RT after new game is selected.

[Ghabry]

ResizeSubcommandPath was simple enough, so I reimplemented it instead of using a trampoline: https://github.com/EasyRPG/regression-tests/commit/a42e2a10a013f26d19416e34d8003bf37144a70a#diff-e125c545e989d33bb0cec3b72a2bbe19

__attribute__((regparm(1/2/3))) is nice because it matches the pascal calling convention.

[Ghabry]

Open problem (maybe is Wine specific, needs testing): Sometimes inputs are lost, making the replay desync. Good that RPG_RT recordings must be only correct once, so can just repeat it until it works.

[Ghabry]

For the ultimate testing environment this needs a DLL that is injected into any RPG_RT.exe. But lets focus on DynRPG for now because it is convenient.

Required hook points. I wonder if they look similar in all RPG_RT. This would allow "byte scanning" for autopatching of any Runtime:

• onStartup (could use DllAttach)
• onNewGame

The following addresses are required (with "onNewGame" 10 hooks/addresses are needed):

• RPG::actors array for "touching" the states
• The RNG function
• UpdateInput that is called inside of the main loop
• ResizeSubcommand and Realloc
• The function that sets the input timer (for writing a oneshot flag)
• The input timer function for manual invocation
• RPG::system for frameCounter and scene
• saveFile of SceneFile

[fmatthew5876]

If all we need are a set of addresses, it should not be hard to create a single file database of binary hash -> hook addresses. Then your test dll could be made to work generally with any supported RPG_RT binary version.