MantisBT - Zandronum
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0000970Zandronum[All Projects] Suggestionpublic2012-08-13 16:292018-09-30 20:22
ReporterBloax 
Assigned ToDusk 
PrioritynormalSeverityfeatureReproducibilityalways
StatusclosedResolutionfixed 
PlatformMicrosoftOSWindowsOS VersionXP/Vista/7
Product Version98d 
Target Version1.1Fixed in Version1.1 
Summary0000970: [Demos] Playback requirements identification improvements
DescriptionWhat a mouthful, indeed.

But thing is, it's incredibly hard to identify what a demo needs to play at the moment.

Something like displaying the current version from the demo's side;
say "Demo was recorded in: 98d-something" as the demo is loaded would help a huge lot with identifying what's needed.

Because right now I have a bunch of demos from late 2009, and what help do you get to identify the required version?

"CLIENTDEMO_ProcessDemoHeader: Expected CLD_DEMOSTART."

..I think you get where I'm going with this.

As another closely related issue, displaying the currently loaded game wads (if it's somehow possible to exclude autoload ones) in the same way would also help dramatically.

Because say, if instead of this: "2009.10.03_16.14.30_duel16-rc1.cld"

I had this: "200910.03_16.14.30_dueldemo1.cld"
You'd have a hard, hard time finding the ONE right version of the EXACT right wad(s).
And even there we have a problem in case there are multiple ones! Oh dear.

But yes, food for thought. I don't expect this any time soon, since I know demos are one big mess.
But boy, this stuff would surely help.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
related to 0000147closed Watermelon if demo doesn't play, print which version of Skulltag the demo was recorded with 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2012-08-13 16:29BloaxNew Issue
2012-08-17 13:45DuskAssigned To => Dusk
2012-08-17 13:45DuskStatusnew => assigned
2012-08-17 13:46DuskNote Added: 0004382
2012-08-17 14:32DuskNote Edited: 0004382bug_revision_view_page.php?bugnote_id=4382#r2406
2012-08-17 14:36DuskStatusassigned => needs review
2012-08-18 19:21Torr SamahoNote Added: 0004392
2012-08-18 23:04DuskNote Added: 0004396
2012-08-18 23:04DuskStatusneeds review => assigned
2012-09-16 01:55DuskNote Added: 0004679
2012-09-16 01:55DuskStatusassigned => needs review
2012-09-16 07:11Torr SamahoNote Added: 0004681
2012-09-16 07:31Torr SamahoStatusneeds review => feedback
2012-09-16 09:38Torr SamahoNote Edited: 0004681bug_revision_view_page.php?bugnote_id=4681#r2536
2012-09-16 10:07DuskStatusfeedback => assigned
2012-09-16 10:08DuskNote Added: 0004685
2012-09-16 10:19Torr SamahoNote Added: 0004688
2012-09-16 10:48DuskNote Added: 0004690
2012-09-16 10:48DuskStatusassigned => needs review
2012-09-16 10:49DuskNote Edited: 0004690bug_revision_view_page.php?bugnote_id=4690#r2538
2012-09-16 11:23Torr SamahoNote Added: 0004693
2012-09-16 11:23Torr SamahoStatusneeds review => feedback
2012-09-16 12:02DuskNote Added: 0004699
2012-09-16 12:03DuskNote Edited: 0004699bug_revision_view_page.php?bugnote_id=4699#r2540
2012-09-16 12:44Torr SamahoNote Added: 0004702
2012-09-16 13:14DuskNote Added: 0004704
2012-09-16 13:15DuskNote Edited: 0004704bug_revision_view_page.php?bugnote_id=4704#r2544
2012-09-16 13:26Torr SamahoNote Added: 0004706
2012-09-16 14:06DuskNote Added: 0004707
2012-09-16 14:08DuskNote Edited: 0004707bug_revision_view_page.php?bugnote_id=4707#r2547
2012-09-16 14:09DuskNote Edited: 0004707bug_revision_view_page.php?bugnote_id=4707#r2548
2012-09-16 14:10DuskNote Edited: 0004707bug_revision_view_page.php?bugnote_id=4707#r2549
2012-09-16 14:31DuskNote Edited: 0004707bug_revision_view_page.php?bugnote_id=4707#r2550
2012-09-16 16:56Torr SamahoNote Added: 0004708
2012-09-16 16:57Torr SamahoNote Edited: 0004708bug_revision_view_page.php?bugnote_id=4708#r2552
2012-09-16 16:57Torr SamahoNote Revision Dropped: 4708: 0002551
2012-09-16 17:57DuskNote Added: 0004712
2012-09-16 18:03Torr SamahoNote Added: 0004713
2012-09-20 10:53DuskNote Added: 0004718
2012-09-20 10:53DuskStatusfeedback => needs testing
2012-09-23 17:06WatermelonNote Added: 0004755
2012-09-23 18:34Torr SamahoNote Added: 0004766
2012-09-23 20:00unknownnaNote Added: 0004776
2012-09-23 20:01unknownnaRelationship addedrelated to 0000147
2012-09-23 20:24DuskNote Added: 0004781
2012-09-23 21:55DuskNote Edited: 0004781bug_revision_view_page.php?bugnote_id=4781#r2593
2012-12-18 20:44DuskTarget Version => 1.1
2013-04-06 14:11ArcoNote Added: 0006247
2013-04-06 16:44DuskNote Added: 0006252
2013-04-06 16:44DuskStatusneeds testing => resolved
2013-04-06 16:44DuskFixed in Version => 1.1
2013-04-06 16:44DuskResolutionopen => fixed
2018-09-30 20:22Blzut3Statusresolved => closed

Notes
(0004382)
Dusk   
2012-08-17 13:46   
(edited on: 2012-08-17 14:32)
I had planned on getting demo WADs stored in the demo rather than relying on the filename for them.. guess it's about time to get that done.

As for >"CLIENTDEMO_ProcessDemoHeader: Expected CLD_DEMOSTART.", I don't know what to do about this. The format obviously changed but I don't know the details of the older demo formats.

EDIT: OK that's done now. I left the changelog entry out for now - if Torr wants this in 1.0 I'll add it and commit this to a new head.
(0004392)
Torr Samaho   
2012-08-18 19:21   
If we add this kind of wad check, we should also consider checking everything. For instance the order in which the wads are loaded may be very important. Your check also allows the player to load more wads that may break the demo. Do we want the check to allow this kind of things? We could also just let the check print visible warnings instead of erroring out.

BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames.
(0004396)
Dusk   
2012-08-18 23:04   
Quote
Your check also allows the player to load more wads that may break the demo.

I was doing that to allow the player to auto-load wads without breaking the demo but that can actually be checked for..

Quote
For instance the order in which the wads are loaded may be very important.

Hmm. Maybe use the authentication checksum for the actual check and print wad list if the check fails?

Quote
BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames.

Hmm. Probably. :P
(0004679)
Dusk   
2012-09-16 01:55   
Alright, how's this?
(0004681)
Torr Samaho   
2012-09-16 07:11   
(edited on: 2012-09-16 09:38)
Much better, but

    ULONG ulWADCount = NETWORK_ReadShort( &g_ByteStream );

    // Read the names of WADs and store them in the array
    FString WadNames[ulWADCount];

doesn't work. It doesn't even compile under VC++ as the size of C arrays has to be known at compile time. As I said earlier, use a TArray to store the strings, i.e. replace FString WadNames[ulWADCount] by TArray<FString> and use "Push" to populate the array.

EDIT: BTW: I think putting dates in the comment is pointless. They are bound not to be updated when something is changed, make things less readable and it's usually easier to use "hg annotate" to find out when a change was made.
(0004685)
Dusk   
2012-09-16 10:08   
Hm. worked fine in GCC...
(0004688)
Torr Samaho   
2012-09-16 10:19   
I'm pretty sure that the C standard only requires arrays whose size is known at compile time to work. VC++ 2005 definitely doesn't compile this.
(0004690)
Dusk   
2012-09-16 10:48   
(edited on: 2012-09-16 10:49)
Changed into a TArray

Also lifted dates from comments.
(0004693)
Torr Samaho   
2012-09-16 11:23   
The code looks good now, but I just noticed a problem when testing your latest patch: The authentication doesn't check the maps at all. So you can still play a demo that was recorded on the completely wrong map since g_lumpsAuthenticationChecksum only checks protected non-map lumps.
(0004699)
Dusk   
2012-09-16 12:02   
(edited on: 2012-09-16 12:03)
That's certainly a bit problematic. Should there be some map checksum kinda thing that would be the checksum of every map loaded, and have that checksum be stored after the network checksum in the demo?
(0004702)
Torr Samaho   
2012-09-16 12:44   
Doing a per map checksum would be the way that's most consistent with how clients and server do the authentication. The client could save the hashes it sends to the server in the demo and then check the maps while playing the demo to the stored hashes.
(0004704)
Dusk   
2012-09-16 13:14   
(edited on: 2012-09-16 13:15)
But then you could run into authentication errors when the demo has already started up. Could get a little frustrating when the demo suddenly bombs out.
(0004706)
Torr Samaho   
2012-09-16 13:26   
That's also true. From the top of my head some alternatives are
- only check the first map (bombing out is replaced by an unwatchable demo on problematic maps which is not really less frustrating)
- build a single MD5 hash of all loaded wads (taking into account the order in which the maps are loaded, similar to how g_lumpsAuthenticationChecksum is built) and check this hash
- check the MD5 hash of all loaded wads and their loading order
With the latter two you should ignore the auto loaded wads (g_lumpsAuthenticationChecksum takes care of this)
(0004707)
Dusk   
2012-09-16 14:06   
(edited on: 2012-09-16 14:31)
Or maybe this?
'http://pastebin.com/VMryWfKf [^]'

Based it off listmaps and auto-compatibility code.

EDIT: Using NETWORK_GenerateMapLumpMD5Hash (mdata, mdata->lumpnum, sum); seemed to get me d41d8cd98f00b204e9800998ecf8427e for every map entry. Maybe I'm using it wrong?
EDIT 2: ... I certainly am... but I dunno is it worth getting it to work properly when the bit from compatibility.cpp works fine in substitution as well.

EDIT 3:
OK it worx (aside from the style I forgot to fix up..)
Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map?
(0004708)
Torr Samaho   
2012-09-16 16:56   
(edited on: 2012-09-16 16:57)
Quote from Dusk
OK it worx (aside from the style I forgot to fix up..)

Seems to work nicely. What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it.

Quote from Dusk
Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map?

This would kill Freedoom/Doom network compatibility completely (now it just kills it for demos, but at least it can be worked around with demo_pure 0 which should be sufficient). Also it's not necessary to prevent somebody from joining when a map differs between client and server that will not be played anyway. The current system works fine and we don't really gain anything by replacing it (except that the startup will be a slower to a still unknown degree and a slight code simplification).
(0004712)
Dusk   
2012-09-16 17:57   
Quote
This would kill Freedoom/Doom network compatibility completely

Hm, that's right.

Quote
What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it.

That is done.
If this is good now, I'll collect this into a new single commit that can be pulled up.
(0004713)
Torr Samaho   
2012-09-16 18:03   
Looks good :).
(0004718)
Dusk   
2012-09-20 10:53   
This was pulled during tracker downtime.
(0004755)
Watermelon   
2012-09-23 17:06   
How can I test this? Do I need to create a demo with the latest version and then check it against different wads or another version?
(0004766)
Torr Samaho   
2012-09-23 18:34   
You can use this binary. And you guessed right, you need to record demos with the new binary and then try to play them with the correct and with wrong wads.
(0004776)
unknownna   
2012-09-23 20:00   
Will the console tell you which version of Zandronum the demo was recorded with if you try to play an old demo with a newer version of Zandronum?

It seems that you're still able to play demos even if you don't load all files, e.g., music files.
(0004781)
Dusk   
2012-09-23 20:24   
(edited on: 2012-09-23 21:55)
The thing uses special lump and map collection authentication for the check. Loading protected lumps should cause it to fail authenticating the demo. However, music files are not protected, thus leaving music out or loading extra music should not cause authentication failures.
(0006247)
Arco   
2013-04-06 14:11   
Old demos from Skulltag cannot be played, as there's an authentication error that's associated with the iwad. The new demos that are recorded with v1.1 can only play if Zandronum has access to the correct wads.
(0006252)
Dusk   
2013-04-06 16:44   
That sounds like correct behavior, then. Demos recorded with older versions never worked in Skulltag or Zandronum so that's not a bug as much as a missing feature.