replays bug(s)

Open-source port of PoP that runs natively on Windows, Linux, etc.
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

Norbert wrote:This replay file gives a segmentation fault for me.
(Probably unsurprising, but just for the record.)
Does it however work for you if you apply the changes from this commit?
https://github.com/Falcury/SDLPoP/commi ... e2032474bd

I'll look at the debug code as soon as I can.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: replays bug(s)

Post by Norbert »

Falcury wrote:Does it however work for you if you apply the changes from this commit? [...]
Yes, with those changes it works for me.
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: replays bug(s)

Post by David »

Falcury wrote:Does it however work for you if you apply the changes from this commit?
https://github.com/Falcury/SDLPoP/commi ... e2032474bd
I think you should make a pull request from that.
Falcury wrote:I have attached a version of Norbert's test replay where I have truncated the size_t variables by four bytes using a hex editor (so it should work on 32-bit versions of SDLPoP).
I did the same with the two other replays from https://github.com/NagyD/SDLPoP/issues/111
See attachment.
I could reproduce the bug with these.
fixed_replays.zip
(1.3 KiB) Downloaded 85 times
And, I could reproduce the bug with a replay recorded by myself.
This could happen no matter how I start the recording: from the command line or after starting the level with Ctrl+Tab.

EDIT:
I added some debug printouts into prandom().
It seems that the saved random seed (that is in the P1R file) appears quite late in the output.
Both when recording and when replaying.
However, from that on the sequence seems identical...

Code: Select all

word __pascal far prandom(word max) {
	printf("before: random_seed = %u\n", random_seed);
	if (!seed_was_init) {
		// init from current time
		random_seed = time(NULL);
		seed_was_init = 1;
	}
	printf("prandom(%d)\n", max);
	random_seed = random_seed * 214013 + 2531011;
	printf("after: random_seed = %u\n", random_seed);
	return (random_seed >> 16) % (max + 1);
}
EDIT:
Hey, this is very odd...
The random seeds match in record and replay, but the distance in autocontrol_guard_kid_armed() is different!
Here, guard_advance() or guard_strike() might be called based on the value of distance.
And this leads to divergence...

I attached a modified source that prints out various debug info for comparison, and a sample replay with outputs.
replay_debug.zip
(250.53 KiB) Downloaded 88 times
The next question is obviously: Why is the distance different?
Maybe I was already pressing the right arrow while some music was playing, and music is not perfectly synced?
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

Pull request:
https://github.com/NagyD/SDLPoP/pull/112

I am still flummoxed on the desynchronization issue, though... although Norbert's test case helps to isolate the problem a bit.
It seems to have something to do with the immobilization of the kid while the kid is crouched at the start of level 1. But I can't seem to find the exact source of the problem.
David wrote:The next question is obviously: Why is the distance different?
Maybe I was already pressing the right arrow while some music was playing, and music is not perfectly synced?
Sorry I didn't see your edit in time. Yes, that's what I thought as well. Although one time when I tried it, the kid seemed to hop from his (immobilized) position in the replay, which shouldn't be possible at all?? Edit: Scratch that, hopping away from the start position is simply possible, I must have been confused with the way you can't control the kid directly after taking fall damage.

Edit: Hey, maybe we could track down the cause of the bug if we also print out the kid's frame number and/or sequence table offset, for each game tick in the replay?
I cannot work on this today...
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

David wrote:Maybe I was already pressing the right arrow while some music was playing, and music is not perfectly synced?
On closer examination I think David's idea here is the correct one.
The kid always stands up *exactly* when the music stops.
The relevant code is in control_crouched() (seg005.c):

Code: Select all

// seg005:02EB
void __pascal far control_crouched() {
	if (need_level1_music != 0 && current_level == 1) {
		// Special event: music when crouching
		if (! check_sound_playing()) {
			if (need_level1_music == 1) {
				play_sound(sound_25_presentation); // presentation (level 1 start)
				need_level1_music = 2;
			} else {
				need_level1_music = 0;
				printf("Music stopped at tick %d\n", 719 - rem_tick); // added
			}
		}
	} else {
		need_level1_music = 0;
		if (control_shift2 < 0 && check_get_item()) return;
		if (control_y != 1) {
			seqtbl_offset_char(seq_49_stand_up_from_crouch); // stand up from crouch
		} else {
			if (control_forward < 0) {
				control_forward = 1; // disable automatic repeat
				seqtbl_offset_char(seq_79_crouch_hop); // crouch-hop
			}
		}
	}
}
I added the printf() statement on line 290 to see at which tick the music stopped.
Normally, it seems the music usually stops at either tick 38 or 39 on my machine (for original level 1; for the test level it's a bit later because the sliding door sound has to finish before the music starts). However, this is not entirely stable and may sometimes be a bit earlier or later.
Still not sure what is going on here, but this is at least an source of inconsistencies, which could logically cause the bug...
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: replays bug(s)

Post by Norbert »

Probably nothing should rely on music durations.
Not the slow fall either, which, if I'm not mistaken would fix another replay issue.
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

Yeah, I agree. I think a possible way to fix this would be to add a 'special move' MOVE_EFFECT_END that would explicitly cancel out the immobilization and feather effects, and cut off the sound if it's still playing. (And at the same time, the sound duration should be ignored while replaying)

Edit: I think I fixed the bug! :)
This is the commit:
https://github.com/NagyD/SDLPoP/pull/11 ... dc6d4b717b
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

The replay format is now like this:

Code: Select all

HEADER
magic number (3 bytes) = 0x50 0x31 0x52 (ASCII: "P1R")
replay format version number (1 byte) = 100 
    (current version number of the replay format)
replay format deprecation number (1 byte) = 1 
    (can be incremented to break backwards compatibility gracefully in the future)
creation time (8 bytes): Unix time (seconds since 1970) at capture
length of the custom levelset name (1 byte) (if zero, the replay uses the original levels)
levelset name: name of the custom levelset (i.e. the folder from which to load custom assets)

SAVESTATE
savestate size (4 bytes)
savestate: currently 0xA87 bytes

REPLAY OPTIONS
(this is split into 5 sections)
size of the options section 'features' (4 bytes)
options section 'features'
size of the options section 'enhancements' (4 bytes)
options section 'enhancements'
size of the options section 'fixes' (4 bytes)
options section 'fixes'
size of the options section 'custom_general' (4 bytes)
options section 'custom_general'
size of the options section 'custom_per_level' (4 bytes)
options section 'custom_per_level'

REPLAY DATA
start level (2 bytes): level number where the replay starts
saved random seed (2 bytes)
number of replay ticks (2 bytes): length of the replay in ticks
moves (1 byte per tick)

Moves are encoded as follows:
struct {     
    sbyte x : 2;
    sbyte y : 2;
    byte shift : 1;
    byte special : 3; (1 = restart level, 2 = effect ends / music stops)
};
One thing that's not yet in the format is a way to tell apart different implementations/forks of SDLPoP. For instance, if someone forks SDLPoP and modifies the format and/or version numbers of the replay format, there would be compatibility issues between the different forks. Ideally, the game should be able to recognize that a replay is "foreign" and not attempt to play it.
So we could add a "unique class identifier", just after the magic number perhaps? Two bytes should be enough, or four perhaps? For normal SDLPoP, this identifier could just be zero, while for a modded SDLPoP one could pick some random identifier which is not zero.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: replays bug(s)

Post by Norbert »

Thanks for documenting that.
Maybe also include the name (and version) of the program that created the replay.
Also, maybe it's better not to include all these option settings?
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: replays bug(s)

Post by David »

Falcury wrote:Edit: I think I fixed the bug! :)
Thank you!
Merged: https://github.com/NagyD/SDLPoP/commits/master
Falcury wrote:On closer examination I think David's idea here is the correct one.
I'm glad I could help! :)
It was just a guess but then it seems it was correct.

Problem: It seems that I can't play back replays any more!
I could fix the crash on new-format replays: https://github.com/NagyD/SDLPoP/commit/ ... 361ad3d6bc
But it still crashes on old-format replays.
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

I added the "unique number" + "name of the program that created the replay" to the format in these commits:

https://github.com/NagyD/SDLPoP/pull/11 ... 485fc33f33
https://github.com/NagyD/SDLPoP/pull/11 ... 808b45b118

New format:

Code: Select all

HEADER
magic number (3 bytes) = 0x50 0x31 0x52 (ASCII: "P1R")
replay format class number (2 bytes): unique identifier that can be changed to distinguish different implementations / forks of SDLPoP
replay format version number (1 byte) = 100 
    (current version number of the replay format)
replay format deprecation number (1 byte) = 1 
    (can be incremented to break backwards compatibility gracefully in the future)
creation time (8 bytes): Unix time (seconds since 1970) at capture
length of the custom levelset name (1 byte) (if zero, the replay uses the original levels)
levelset name: name of the custom levelset (i.e. the folder from which to load custom assets)
length of the name of the program that created the replay (1 byte)
name of the program that created the replay (e.g. "SDLPoP v1.17")

SAVESTATE
savestate size (4 bytes)
savestate: currently 0xA87 bytes

REPLAY OPTIONS
(this is split into 5 sections)
size of the options section 'features' (4 bytes)
options section 'features'
size of the options section 'enhancements' (4 bytes)
options section 'enhancements'
size of the options section 'fixes' (4 bytes)
options section 'fixes'
size of the options section 'custom_general' (4 bytes)
options section 'custom_general'
size of the options section 'custom_per_level' (4 bytes)
options section 'custom_per_level'

REPLAY DATA
start level (2 bytes): level number where the replay starts
saved random seed (2 bytes)
number of replay ticks (2 bytes): length of the replay in ticks
moves (1 byte per tick)

Moves are encoded as follows:
struct {     
    sbyte x : 2;
    sbyte y : 2;
    byte shift : 1;
    byte special : 3; (1 = restart level, 2 = effect ends / music stops)
};
Note that implementation_name (name of the program that created the replay) is only informative; SDLPoP will only report an error message if the number (replay_format_class) is different from its own.

Added some more informative error messages in the third commit:
https://github.com/NagyD/SDLPoP/pull/11 ... 6fd4b5dfdb

I also bumped REPLAY_FORMAT_CURR_VERSION and REPLAY_FORMAT_MIN_VERSION from 100 to 101. This breaks backward compatibility again (for the last time maybe, for now?)
David wrote:Problem: It seems that I can't play back replays any more!
I could fix the crash on new-format replays: https://github.com/NagyD/SDLPoP/commit/ ... 361ad3d6bc
But it still crashes on old-format replays.
Old replays wouldn't work anymore, but a crash is obviously not supposed to happen...
Hm, maybe because this is only a warning, and not an error:
https://github.com/NagyD/SDLPoP/pull/11 ... 52020fR707

Edit:
Possible fix in this commit:
https://github.com/NagyD/SDLPoP/pull/11 ... 242fae5c6e

Edit:
Two short test replays:
test_replays.zip
(1.84 KiB) Downloaded 78 times
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: replays bug(s)

Post by Norbert »

Falcury wrote:Edit: I think I fixed the bug! :)
I only saw your edit when reading David's response.
Good news, I hope you're right.

By the way, sort of on-topic, I saw the last MININIM release also mentions replays.
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: replays bug(s)

Post by David »

I hate to say this, but old replays still crash SDLPoP if I try to open one with Tab.
But only if I start SDLPoP without giving a *.P1R filename.
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: replays bug(s)

Post by Falcury »

David wrote:I hate to say this, but old replays still crash SDLPoP if I try to open one with Tab.
But only if I start SDLPoP without giving a *.P1R filename.
Hm... it should simply skip over anything it doesn't recognize in the replays folder... I cannot seem to easily reproduce this. Which files can cause the crashing for you?
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: replays bug(s)

Post by David »

Falcury wrote:Which files can cause the crashing for you?
See attachment.

If I pass it in the command line, then it just prints an error message.
If I don't pass any *.P1R filename then it crashes.
Attachments
replays_old.zip
(693 Bytes) Downloaded 72 times
Post Reply