Extensible demo format + "PR+UM" signature #107
Conversation
Address concerns from code review of extensible demo format
XaserAcheron
changed the title
|
Just merged in some changes from Altaz that should address all the concerns -- apologies for the delay; GitHub chose not to notify me about this for some reason. o_O |
|
It's been a bit since the last comments. Is there any additional hold-up left on this? |
It's controversial, to say the least. And the project owner @coelckers has yet to give a sign of acknowledgement. |
|
I was under the impression all the issues that caused any controversy have been ironed out, and I probably wouldn't wait on Graf. I don't recall him acknowledging this project for at least half a year. |
|
This PR is basically the thing that is stopping this port from being accepted as an "official" successor to prboom-plus mainline, and there are no unaddressed issues here. What's the hold-up? |
I somehow feel addressed by this complaint, because apparently I am the only one left with commit rights who is still actively interested in this port. First of all, this patch is very intrusive, given that it all started with the insertion of a single Second, this implements what someone else - who is not the author of this fork and thus not the author of the UMAPINFO reference implementation - wants UMAPINFO demo headers to look like. This, without a single sign of acknowledgement by the original author. Third, there is still an open question regarding the nesting of demo signatures and the necessity of the MBF demo signature. There are questions asked by @Ferk and @kraflab in July and I requested elaboration just yesterday, because - frankly - I fail to understand this part of the code as well. I understand there is probably not much that can be done about point 1 if we want to keep this level of complexity. Same for point 2 which is nothing that anyone of you could be blamed for. What I'd really like to have, though, is confirmation (a) that UMAPINFO demos can be recorded and played back with any complevel and thus embed demos in any other format, (b) that this is forward and backward compatible with The Eternity Engine, and (c) that UMAPINFO demos will be detected and rejected by non-UMAPINFO ports (such as Chocolate). |
|
Would it help if we split this into two pieces?
In particular for (1) it would probably help if there was a (non code) specification of the format to look at to answer the questions raised about compatibility and make sure everyone is on the same page. Maybe that exists on the forum already and could just be linked. |
|
@kraflab nice idea, though I somehow expected (2) before (1). |
There was a problem hiding this comment.
I gave this patch another review.
In general, the part in G_BeginRecording() looks good to me, but raises a question as to when the special UMAPINFO treatment gets triggered (if UMAPINFO lump found and loaded vs. if demo start map has UMAPINFO changes applied).
Then there is a either misconception about MBF using 0xff as its demo version number, or I am missing something.
Third, there is a copy/paste bug in the G_ReadDemoHeaderEx() part that needs to be addressed - and that leaves me wondering how much this code has actually been tested in real life scenarios.
Please address these issues before we can proceed, thanks!
prboom2/src/g_game.c
Outdated
| demostart = demo_p = malloc(1000); | ||
| longtics = 0; | ||
|
|
||
| if (umapinfo_loaded) | ||
| // ano - jun2019 - add the extension format if needed | ||
| if (gamemapinfo) |
There was a problem hiding this comment.
Why is this changed from umapinfo_loaded to gamemapinfo here? AFAIUI, gamemapinfo is only set if there is UMAPINFO information available for the current map, whereas umapinfo_loaded is set a soon as a UMAPINFO lump is found and loaded. But what if we start a demo from a map without UMAPINFO changes and then migrate to UMAPINFO affected maps during the course of recording?
There was a problem hiding this comment.
Good catch -- it indeed oughta be umapinfo_loaded. I whipped up a test wad which contained UMAPINFO for maps 2, 3, and 4, recorded a demo starting from MAP01, and it skipped writing the extension header entirely. Got a fix incoming.
prboom2/src/g_game.c
Outdated
| return NULL; | ||
|
|
||
| // we check for the PR+UM signature as mentioned. | ||
| // MBF and Eternity Engine also use 255 demover, with other signatures. |
There was a problem hiding this comment.
I think MBF uses 203 as its demo version number. Please double-check and correct here if this is true or tell me if I'm wrong. ;)
prboom2/src/g_game.c
Outdated
| I_Error("G_ReadDemoHeader: Unable to determine map for UMAPINFO demo."); | ||
| } | ||
|
|
||
| episode = (episode * 10) + (cur - '0'); |
There was a problem hiding this comment.
This should be map here instead of episode, right?
|
Finally had some time to sit down and play around with this some more -- tl;dr: made all the requested changes. I put together a quick test wad while I was at it to repro & fix the troubles: The wad contains a couple of maps, + a UMAPINFO file that defines maps 02, 03, 33, and 04 (but not MAP01), and sets up a branching path to MAP03/MAP33 via secret exit on MAP02. I recorded |
|
@XaserAcheron Thank you very much for taking care of the requested changes! One last thing that still annoys me is the completely redundant parsing of the ExMy/MAPxy level name. I know, the reason is that it should be possible in the future to have non-standard map names defined by UMAPINFO. However, the current implementation does not support this and even UMAPINFO's own Anyway, it seems to work as intended now and I am not going to argue about this PR anymore. Merging, merry (late) Christmas everybody! 🎄 |
Hey y'all,
There's been some talk about extending the demo format + signature in the Doomworld thread recently, so I figured for convenience's sake, I'd make a PR for anotak's implementation of this. It's gotten some eyes on it in the past, code-review-wise but we may as well get the ball rolling on getting it submitted for real.
More comments on this (plus a quote from Graf showing support for the idea, in case that was in question) on DW here.