Record advance time #457

NQNStudios · 2024-09-25T19:02:52Z

Record and replay actions from Recharge spell #422
Implement --verbose flag for replay recording, which records internal testing actions (for now just advance_time)
This PR I think covers making the vast majority of action cases perform advance_time() identically to original runtime behavior. I would say it's highly likely that I missed edge cases, but this version of the code is much more correct.
Also of note: obsessively matching the behavior of advance_time() while correct and good, is not entirely necessary for replay integrity as long as action replays always handle the did_something flag correctly.

To test this and find every discrepancy, I used these two replays which together contain one of every action (I'm pretty sure):

Archive.zip

There are some major caveats to those 2 replays, which is why I am not staging them in the repository:

They were both recorded with different builds, each in a modified state
In the first one, I deleted some actions at the end which led to the crash "Ask About" text input crash #455
As I was testing and fixing, I changed the way some actions record themselves. Rather than re-record the whole logs, I would edit the log to match the changing behavior (but I could have made a mistake).
They take very very long.

Some things about these replays are cool:

They recreate pretty long play sessions, appearing to match the state of the original playthrough perfectly, including in combat (which is fun to watch)
You can see the replays reproduce some UI bugs which would be impossible to check with unit testing
If I ever demo OpenBoE at a public event, I'll probably make replays to have looping when no one's actively playing it :)

This is a wibbly wobbly PR which I'm afraid rebasing for cleaner history, would get too hairy. So I haven't tried squashing anything yet. I also don't think it would be proper to squash the whole PR into one commit, because it shows the evolution of the --verbose flag and my reasoning for it. All the contiguous "fix some cases" commits could probably be squashed together easily, though.

My next recording system PRs after this one will do the following to address the dangling problems:

Add two folders, test/replays/short and test/replays/long
Add a much shorter log that records one of everything FROM A CLEAN REPO STATE, WITHOUT MANUAL MODIFICATIONS
Add a bash script test/replays.sh which runs the replays, with the options to fail fast and/or skip the long ones.
On success of a comprehensive run of all replays, this script will update a file test/replays/success_{platform}.txt which records the latest gitrev info unless the repository has been modified. This file can be staged, and potentially the CI for release builds could check for its existence and assert that a pull request comes with a recent success SHA for all 3 platforms (though this requires a dev to have access to all of Mac/Windows/Linux, which I do for now, but obviously not everyone will and my computers might break some day.)
Get the replay test to pass on all 3 platforms so that success files can exist for all 3.

NQNStudios · 2024-09-25T19:08:28Z

That win-scons one failed for github action reasons. Needs to re-run

CelticMinstrel · 2024-09-25T23:20:55Z

though this requires a dev to have access to all of Mac/Windows/Linux, which I do for now

I technically have access to all 3 too, though my Windows/Linux computer has been turned off for quite awhile and probably needs servicing.

src/tools/replay.cpp

CelticMinstrel · 2024-09-25T23:29:57Z

src/game/boe.specials.cpp

@@ -1282,6 +1282,7 @@ bool use_space(location where) {
 // Note ... if this is a container, the code must first process any specials. If
 // specials return false, can't get items inside. If true, can get items inside.
 // Can't get items out in combat.
+// TODO this always returns false. Why?


I suspect it's supposed to return s1.

src/tools/replay.cpp

src/game/boe.main.cpp

CelticMinstrel · 2024-09-25T23:38:39Z

This one looks wrong: 762feec

One other that looked wrong is handle_alchemy, but I'm not sure of that – I think it should advance time if you make a potion, but it certainly shouldn't advance time if you open the alchemy menu and then cancel. Were both those cases tested?

There are a couple of TODO notes that need removing now. One is the one you can see in 47b4456 and the other is the original TODO note about validating advance_time.

NQNStudios · 2024-09-25T23:47:10Z

This one looks wrong: 762feec

I believe it's correct -- screen shifting needs to call advance_time() with need_redraw true but did_something false

NQNStudios · 2024-09-25T23:50:32Z

One other that looked wrong is handle_alchemy, but I'm not sure of that – I think it should advance time if you make a potion, but it certainly shouldn't advance time if you open the alchemy menu and then cancel. Were both those cases tested?

Oh, I was definitely rushing though when I recorded handle_alchemy. I have to redo that one and make sure to get both cases

CelticMinstrel · 2024-09-25T23:52:02Z

This one looks wrong: 762feec

I believe it's correct -- screen shifting needs to call advance_time() with need_redraw true but did_something false

Ah, I guess that half makes sense. It suggests advance_time isn't quite the right name for it, but close enough I guess.

Oh, I was definitely rushing though when I recorded handle_alchemy. I have to redo that one and make sure to get both cases

What about other dialog things that can be cancelled? Like casting a spell, or deleting a PC… or giving a stackable item with charges…

NQNStudios · 2024-09-26T00:01:22Z

No, I didn't comprehensively test canceling cancelable actions. That actually is probably gonna reveal new bugs and discrepancies

EDIT: ...or not. Because --verbose just records what happens. Actually I would expect those cases to just work. We'll see.

NQNStudios · 2024-09-26T00:05:14Z

Ah, I guess that half makes sense. It suggests advance_time isn't quite the right name for it, but close enough I guess.

Right. advance_time only actually advances time if an action set did_something to true. The other two flags just redraw the screen and print out buffer messages. did_something is very rarely set separately from the business logic of actions' functions in boe.actions.cpp, which means we possibly had leeway to keep calling advance_time after every action in the first place.

Unless advance_time() changed behaviors in the future. Then we would be glad that we got it right.

NQNStudios · 2024-09-26T00:12:38Z

~~> Because --verbose just records what happens. Actually I would expect those cases to just work.~~

~~I can express this better.~~

~~Actions are calling synchronous dialogs to either complete the action or cancel it. After the synchronous dialog runs, they're calling advance_time.~~

~~At replay-time, the dialogs also run synchronously and complete the action logic (accounting for cancel) before checking advance_time. So it should match without extra work.~~

I think this is completely wrong. I'm too many hours deep on this for today. Not thinking clearly.

NQNStudios · 2024-09-26T16:09:56Z

https://github.com/calref/cboe/pull/457/files#diff-0fb416d013a449a9dcaaf68891fc4bf89204fbb9a69ea747e6c27141026ea24aL424

if this ever triggers a spell being cast outside of combat, it should probably advance time. Inside of combat, I assume the Action Points system would handle advancing the time.

test/replays/short/Issue455.xml

CelticMinstrel · 2024-09-27T02:52:15Z

How is the base64 content formatted when a load game action is recorded? If possible I'd like it to be broken into fixed-length lines (except for the final line). It could maybe use a CDATA section… I think that might be required to preserve the line breaks with the way you've implemented the replay system, though this isn't really a case where whitespace needs to be preserved at all… but a base64 decoder is supposed to ignore whitespace anyway so it shouldn't matter.

CelticMinstrel

Are you planning to finish this anytime soon?

NQNStudios · 2024-11-09T19:55:39Z

I’m coming back from an RSI-induced hiatus and my efforts are a little diffuse across different issues right now. So yes, but I’m distracted.

…

On Sat, Nov 9, 2024 at 1:53 PM Celtic Minstrel ***@***.***> wrote: ***@***.**** commented on this pull request. Are you planning to finish this anytime soon? — Reply to this email directly, view it on GitHub <#457 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATXBKPXRCBCD7PBFRTS7GTZ7ZR25AVCNFSM6AAAAABO3FZO2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRVGU3DONZYGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

NQNStudios · 2024-11-10T01:42:24Z

How is the base64 content formatted when a load game action is recorded? If possible I'd like it to be broken into fixed-length lines (except for the final line). It could maybe use a CDATA section… I think that might be required to preserve the line breaks with the way you've implemented the replay system, though this isn't really a case where whitespace needs to be preserved at all… but a base64 decoder is supposed to ignore whitespace anyway so it shouldn't matter.

The Linux man page for base64 says 76 is the default chars-per-line of base64 output:
https://linux.die.net/man/1/base64

I can't find any definitive standard about how many chars per line there should be, so I'll use 76.

NQNStudios added 25 commits September 23, 2024 15:40

remove duplicate include

a9887ba

record and replay cancelling item target mode

c705775

record and validate advance_time() calls with --advance-time

670ef92

fix some advance_time cases

cbc559b

missing advance_time print last command

730a7bb

after advance_time with wrong args, print last action type

3e2b3ea

fix an advance_time case

8954ce2

fix replay behavior of item stats window buttons

fc67811

fix advancee_time behavior of more actions

fd8058e

discard control_click when tracking last_action_type

c63dce5

fix more advance_time cases

8725288

fix more advance_time calls

a8102da

fix more advance_time calls

11ed073

fix parsing display_spells and display_skills actions

96e3935

fix more advance_time

c877eed

TODO comment on adj_town_look()

091fd87

change --advance-time to --verbose, w/ better replay behavior

efa5893

fix an advance_time case

762feec

fix more advance_time cases

382dfaf

consistent advance_time behavior for display_map key vs. button

ab2cabe

fix more advance_time cases

b60ccaa

fix a new crash at end of replay log

9ef5643

fix more advance_time cases

cc7c8b4

replay load_party() more accurately

081578c

handle last (?) advance_time case

47b4456

replay test bash script (tested, works on mac)

c2ef6e2

CelticMinstrel reviewed Sep 25, 2024

View reviewed changes

src/tools/replay.cpp Outdated Show resolved Hide resolved

CelticMinstrel reviewed Sep 25, 2024

View reviewed changes

src/tools/replay.cpp Outdated Show resolved Hide resolved

CelticMinstrel reviewed Sep 25, 2024

View reviewed changes

src/game/boe.main.cpp Outdated Show resolved Hide resolved

prefix action debug prints

e89f1e9

NQNStudios added 3 commits September 26, 2024 10:41

rename control_click to click_control

147ad4e

remove TODOs

24f49c7

call advance_time for cancel_item_target

2122227

NQNStudios force-pushed the record-advance-time branch from bea9ee1 to 2122227 Compare September 26, 2024 16:07

NQNStudios added 2 commits September 26, 2024 11:32

make cosmetic advance_time() mismatches warnings unless --strict

0eb48d9

thoroughness level 2 - use --strict

e21b1d8

CelticMinstrel reviewed Sep 27, 2024

View reviewed changes

test/replays/short/Issue455.xml Show resolved Hide resolved

CelticMinstrel reviewed Nov 9, 2024

View reviewed changes

make build_scen_headers use cdata

76e76d8

wrap load_party base64 data

3ac948c

CelticMinstrel merged commit 77297c2 into calref:master Nov 10, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record advance time #457

Record advance time #457

NQNStudios commented Sep 25, 2024 •

edited

Loading

NQNStudios commented Sep 25, 2024

CelticMinstrel commented Sep 25, 2024

CelticMinstrel Sep 25, 2024

CelticMinstrel commented Sep 25, 2024

NQNStudios commented Sep 25, 2024

NQNStudios commented Sep 25, 2024

CelticMinstrel commented Sep 25, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024

CelticMinstrel commented Sep 27, 2024

CelticMinstrel left a comment

NQNStudios commented Nov 9, 2024 via email

NQNStudios commented Nov 10, 2024

Record advance time #457

Record advance time #457

Conversation

NQNStudios commented Sep 25, 2024 • edited Loading

NQNStudios commented Sep 25, 2024

CelticMinstrel commented Sep 25, 2024

CelticMinstrel Sep 25, 2024

Choose a reason for hiding this comment

CelticMinstrel commented Sep 25, 2024

NQNStudios commented Sep 25, 2024

NQNStudios commented Sep 25, 2024

CelticMinstrel commented Sep 25, 2024 • edited Loading

NQNStudios commented Sep 26, 2024 • edited Loading

NQNStudios commented Sep 26, 2024 • edited Loading

NQNStudios commented Sep 26, 2024 • edited Loading

NQNStudios commented Sep 26, 2024

CelticMinstrel commented Sep 27, 2024

CelticMinstrel left a comment

Choose a reason for hiding this comment

NQNStudios commented Nov 9, 2024 via email

NQNStudios commented Nov 10, 2024

NQNStudios commented Sep 25, 2024 •

edited

Loading

CelticMinstrel commented Sep 25, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading

NQNStudios commented Sep 26, 2024 •

edited

Loading