Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doom v0.99 demo support. #1207

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

SmileTheory
Copy link
Contributor

This one is like #1205 , but for v0.99.

Note that v1.1 demos very much don't work.

Yet.

Copy link
Member

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive! Where did you get all this from?

@SmileTheory
Copy link
Contributor Author

A decompiler (http://derevenets.com/ ), a hex editor, a memory dump from dosbox, and a whole lot of reading and comparing code. :)

Copy link
Member

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have these probably been float value in v1.0? I find the 900 denominator a bit irritating.

@SmileTheory
Copy link
Contributor Author

Without the 900 denominator they're the exact values, pulled straight out of my memory dump from dosbox, no floats involved.

If you used them directly, they'd be encoded inaccurately when recording a demo. The original 1.0/1.1 divided movement values by 900 when writing a demo and multiplied by 900 when reading them.

To be honest I haven't actually tested recording demos, though.

@SmileTheory
Copy link
Contributor Author

Bleh, I managed to record a v1.0 video that plays out differently in the original exe and in chocolate doom.

Attached, also still trying to figure out why that imp at the end moves differently.

1.zip

@SmileTheory
Copy link
Contributor Author

fa7bcca appears to fix the demo.

gameversion = exe_doom_1_1;

// detect v1.0 from length of demo1 lump
if (demo1 && W_LumpLength(demo1) < 10 * 1024)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be if (demo1 > -1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fb10e90 .

src/doom/d_main.c Outdated Show resolved Hide resolved
Using smaller delta tables instead of huge full-sized ones
@@ -1094,7 +1094,7 @@ static void InitGameVersion(void)
gameversion = exe_doom_1_1;

// detect v1.0 from length of demo1 lump
if (demo1 && W_LumpLength(demo1) < 10 * 1024)
if (demo1 > -1 && W_LumpLength(demo1) < 10 * 1024)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the checks in InitGameVersion() become more and more complex, we should probably consider moving all this away from doom/d_main.c into a separate file?

Copy link
Member

@fragglet fragglet Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, +1 on this. This is my main beef with this PR as it currently stands.

The changes necessary to support v1.0 can be broadly categorized as follows:

  1. Patch some of the normal tables and constants to match v1.0.
  2. Change some of the game logic to match v1.0 (with if (gameversion...) {...}

The problem with (2) is that it can introduce a lot of complexity to the code if we're not careful. So we should try to avoid doing this as much as possible. What can be rearchitected here to put things into the (1) category instead of (2)? Try to get as few new if statements as possible.

My immediate suggestions for how to proceed on this PR are as follows:

  • Make a new file - maybe named something like d_oldversion.c - and try to isolate as much as possible to that file. That includes the "Version-specific differences" switch statement below, and the alternative versions of the trig tables, so we move those out of tables.c.

  • It should be possible to patch the forwardmove[] and sidemove[] tables from the new file too, so we avoid adding that complexity.

  • Define new functions named eg. D_CoarserSine and D_CoarserCosine that will either look up into the coarsesine[] table or the finesine[] table depending on the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d8f6bcf .

@SmileTheory
Copy link
Contributor Author

As an aside, I figured out how the various trig tables are calculated for Doom v1.0 and versions after, and put code for them into https://gist.github.com/SmileTheory/02e6a13bc615efa3b6c31decc0ea049a .

@@ -56,7 +56,9 @@ typedef enum

typedef enum
{
exe_doom_1_2, // Doom 1.2: shareware and registered
exe_doom_1_0, // Doom 1.0: shareware
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in previous PR - this probably affects network protocol compatibility. That doesn't mean I'm against merging this PR, but we probably need to "fork" this enum into a separate net_game_version_t that will preserve the ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d8f6bcf .

gameversion = exe_doom_1_1;

// detect v1.0 from length of demo1 lump
if (demo1 > -1 && W_LumpLength(demo1) < 10 * 1024)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison here (length < 10 * 1024) seems oddly magic. I'd be in favour of making this more specific: can we do this based on a SHA1 hash of the demo contents for example? w_checksum.c already has some code for checksumming lumps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d8f6bcf .

src/tables.c Outdated
@@ -2225,3 +2233,192 @@ const byte gammatable[5][256] =
}
};

const fixed_t coarsesine[320] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the tables will be included in all games, not just the chocolate-doom binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into d_compat.c in d8f6bcf .

if (demo1 > -1 && W_LumpLength(demo1) < 10 * 1024)
gameversion = exe_doom_1_0;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove spurious whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in d8f6bcf .

mobjinfo[MT_MISC7].flags &= ~MF_NOTDMATCH;
mobjinfo[MT_MISC8].flags &= ~MF_NOTDMATCH;
mobjinfo[MT_MISC9].flags &= ~MF_NOTDMATCH;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where a switch() {...} fallthrough is intentional, please add a comment that says something like // fallthrough - otherwise this looks like a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to d_compat.c and changed in d8f6bcf .

src/tables.c Outdated
for (i = 0; i < 66; i += 2)
finesine1_2[finesineto1_0[i]] += finesineto1_0[i+1];

for(i = 0, j = 0, count = 1; count; j += count)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the formatting here is sloppy. Please try to match the style described in HACKING and please also don't introduce new tab characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d8f6bcf .

src/tables.c Outdated
const fixed_t *coarsecosine = &coarsesine[64];

const short finesineto1_0[] = {
455,-1,1080,-1,2268,1,3212,-1,3324,-1,3640,-1,3822,1,4147,1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better expressed as something like this?

const struct {
  unsigned short index;
  short value;
} finesine_to_1_0[] = {
  {455, -1},
  ....
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d8f6bcf .

@@ -1094,7 +1094,7 @@ static void InitGameVersion(void)
gameversion = exe_doom_1_1;

// detect v1.0 from length of demo1 lump
if (demo1 && W_LumpLength(demo1) < 10 * 1024)
if (demo1 > -1 && W_LumpLength(demo1) < 10 * 1024)
Copy link
Member

@fragglet fragglet Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, +1 on this. This is my main beef with this PR as it currently stands.

The changes necessary to support v1.0 can be broadly categorized as follows:

  1. Patch some of the normal tables and constants to match v1.0.
  2. Change some of the game logic to match v1.0 (with if (gameversion...) {...}

The problem with (2) is that it can introduce a lot of complexity to the code if we're not careful. So we should try to avoid doing this as much as possible. What can be rearchitected here to put things into the (1) category instead of (2)? Try to get as few new if statements as possible.

My immediate suggestions for how to proceed on this PR are as follows:

  • Make a new file - maybe named something like d_oldversion.c - and try to isolate as much as possible to that file. That includes the "Version-specific differences" switch statement below, and the alternative versions of the trig tables, so we move those out of tables.c.

  • It should be possible to patch the forwardmove[] and sidemove[] tables from the new file too, so we avoid adding that complexity.

  • Define new functions named eg. D_CoarserSine and D_CoarserCosine that will either look up into the coarsesine[] table or the finesine[] table depending on the version.

@drfrag666
Copy link
Contributor

"In versions of Doom 1.2 and earlier, armor percentage was not limited to 200."
https://doomwiki.org/wiki/Armor_percentage_rollover

@SmileTheory
Copy link
Contributor Author

"In versions of Doom 1.2 and earlier, armor percentage was not limited to 200."
https://doomwiki.org/wiki/Armor_percentage_rollover

This was in the old pull request:

9eae89d#diff-79f9cf84b40c123971d3775511e0a3e3R385

If that's too hard to read, here's a comment:

0e72254#diff-79f9cf84b40c123971d3775511e0a3e3R385

@drfrag666
Copy link
Contributor

Someone commented it at doomworld and i was not aware it was already in.

@drfrag666
Copy link
Contributor

Are 1.1 demos supported already?

@SmileTheory
Copy link
Contributor Author

Are 1.1 demos supported already?

The demos in the Doom 1.1 shareware wad work, but beyond that I haven't tested too much.

@SmileTheory
Copy link
Contributor Author

SmileTheory commented Nov 26, 2019

Are 1.1 demos supported already?

The demos in the Doom 1.1 shareware wad work, but beyond that I haven't tested too much.

Only in Shareware? They don't work in the registered/full version?

In 1.1 registered, DEMO1 and DEMO2 work, but DEMO3 desynchs a bit after getting the chainsaw.

It doesn't seem chainsaw related though, comparing with the original 1.1 exe shows that the cacos on the left are in slightly different positions, and the lost soul that actually causes the desynch takes a few tics longer to attack on the 1.1 exe than with this patch.

I've been investigating this for a while but I haven't been able to figure it out yet.

Fixes Doom 1.1 registered DEMO3.
@drfrag666
Copy link
Contributor

You rule! :)

@SmileTheory
Copy link
Contributor Author

"Wallrunning doesn't work in Doom v1.1 (North-South and East-West walls), except for some diagonal walls for which it's the same as Doom v1.9."

I'd guess that this is a consequence of the half step movement code not being implemented until Doom v1.2. (https://github.com/chocolate-doom/chocolate-doom/pull/1207/files#diff-9b3d1e95726cd7eaef62f94a3d6caf92R158 )

More info at https://doomwiki.org/wiki/Wallrunning#Technical .

@drfrag666
Copy link
Contributor

Okay i've merged this and done some testing. For me it's broken after "Requested changes". You can't start a new game until the first demo plays, the game quits inmediately or after the wipe hence you can't play demo2 or demo3 with playdemo. Besides after the demo ends the wipe is shown twice.
That is with the 1.1 iwad. Does it work for you?

@SmileTheory
Copy link
Contributor Author

Okay i've merged this and done some testing. For me it's broken after "Requested changes". You can't start a new game until the first demo plays, the game quits inmediately or after the wipe hence you can't play demo2 or demo3 with playdemo. Besides after the demo ends the wipe is shown twice.
That is with the 1.1 iwad. Does it work for you?

It works fine with 1.1 registered and 1.1 shareware for me. Leaving it to run moves on to demo2 as normal instead of quitting, and I can start a game at any point.

Are you using cmake to build? I haven't tested cmake since I can never get the thing to work properly. :)

Otherwise, did the branch merge cleanly? The only other thing I can think of is some conflict, unless you're using a clean version of chocolate doom.

@drfrag666
Copy link
Contributor

Sorry, it was a bad merge of "Fix inaccurate v1.0/v1.1/v1.2 sight calculations". Now it works, BTW i've lost my internet connection.

@drfrag666
Copy link
Contributor

I've released a test build with this PR in order to help with old demos here:
https://github.com/drfrag666/chocolate-doom/releases/tag/2.5.0a

@drfrag666
Copy link
Contributor

Uploaded another test build with the latest changes:
https://github.com/drfrag666/chocolate-doom/releases/download/2.5.0b/RUDE-2.5.0b-win32.zip

@tpoppins
Copy link
Contributor

Thank you for the test build, Dr. Frag666!

Here is a set of demos recorded on E2 and E3 of the registered v1.1 in February-March '94: VDOOM2_3.ZIP. It doesn't include a demo for E3M9, and the E3M6 one terminates prematurely due to reaching the default demo size. Of the other 16 demos all but three play to the end with RUDE v2.5.0b.

The desyncing demos are those for E3M1, E3M5 and E3M7. Vanilla v1.1 plays these successfully (i.e. the player reaches the exit and exits the level).

This set should provide some extra testing material.

@drfrag666
Copy link
Contributor

Thanks. These are useful. E3M1 desyncs becouse the door-lift closes too early. E3M5 becouse the bfg doesn't kill a demon near the end. E3M7 becouse the bridge is missing apparently. We'll know more when SmileTheory tries them with his build.

@drfrag666
Copy link
Contributor

I've released a new test build with the latest changes.
https://github.com/drfrag666/chocolate-doom/releases/tag/2.5.0c

@tpoppins
Copy link
Contributor

Good job, SmileTheory: all three demos mentioned above complete successfully now in RUDE Doom 2.5.0c!

@drfrag666: thank you for the bins. Would you email me (tpoppins at juno dot com) please, I can't compile your fork in either CodeBlocks or MSYS2.

@drfrag666
Copy link
Contributor

I've enabled issues, post your problem there and i'll reply ASAP.

@drfrag666
Copy link
Contributor

Any issue left here?

@tpoppins
Copy link
Contributor

tpoppins commented Nov 16, 2022

Yes, there are still a few issues.

  1. "-fast" should be disabled for v1.2 and older because "-fast" was first introduced in v1.4 beta leak (Apr 8 1994). I just submitted a PR that corrects this. Note to self: Prboom-plus incorrectly allows fast monsters with v1.2 compat as well, subject to another PR.

  2. Additionally, there are no "-respawn" and "-nomonsters" in versions 0.99/1.0 and 1.1.

  3. Support for forcing "-respawn" and "-nomonsters" on command line for playback of v1.2 demos is missing. That should be fixed by another PR I just submitted.

  4. The biggest issue for last: not all plats stayed active in v1.2 and older. Apparently it's wrong to lump together raiseAndChange and raiseToNearestAndChange as commit 3fe7123 does.

The following demo on ANTHELL4, recorded with vanilla v1.2, desyncs in the latest RUDE (which incorporates the changes from that commit) because of that.
ANTHELL4-v12-test.zip

Linedef 898 has action special 67 (SR Floor Raise by 32 and Change), which is raiseAndChange. In vanilla v1.2 (and v1.1 and v1.9u) you can activate this switch multiple times; with that commit applied - only once. Separating the raiseAndChange case like this:

		  case raiseAndChange:
	        P_RemoveActivePlat(plat);
		    break;

allows the demo to play back correctly to the end. Latest Choco autobuild plays back the demo properly since it doesn't incorporate that commit's changes.

And then there's that issue with CHURCH.WAD mentioned above.

@tpoppins
Copy link
Contributor

tpoppins commented Dec 23, 2022

The CHURCH.WAD internal demo desync's cause has been identified: sector special 17 (random flicker) was not a valid special until the official v1.4 beta.

Meanwhile, a v0.99/1.0 demo that desyncs with this PR has been found: LEVEL7.LMP in DOOMLVL7.ZIP.

It's a 11:49 UV demo of the IWAD E1M7 that plays back properly to the exit in vanilla. The desync becomes evident around 3:20-3:25. In case it could be relevant, there's a three-second pause at 2:35; removing it doesn't make a difference.

edit: removing the pause (manually, or with LMPCm) makes vanilla desync the same way at the same spot. Some more experimenting followed:

  • recorded a UV Max in vanilla without using pause and it played back fine in my Choco
  • did same thing, this time pausing briefly after taking the yellow key, and the demo desyncs in Choco before you get to the red key
  • same issue with v1.1
  • unable to reproduce in v1.2

So using pause during recording is definitely a factor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants