Thanks everyone for the feedback! I updated the PR with your suggestions 😀 I also fixed an issue where the spacing between Sigil and Sigil II on the episode selection screen looked awkward due to the taller height of the Sigil episode text graphics compared to the default.
ludicrous_peridot wrote on 2023-12-17, 12:49:
dgondos wrote on 2023-12-16, 09:24:
I wanted to use MBF on DOS to play Sigil II so I hacked on some patches on top of it to support it - seems to work OK. Is anyone else using this fork or is everyone using a different patch for ep5/ep6 support?
I think @crvs version is the preferred one as it's changes are eventually getting merged into upstream. Sadly I don't know if this is indeed the latest code that is being reviewed, but it would be interesting to see if your PR would merge successfully if rebased on top of that. Also second concern over Allegro version used, even if for @gerwin's latest bugfixes.
That's a good idea, I can have a look at that.
crvs wrote on 2023-12-18, 06:17:Your PR may need several corrections: […]
Show full quote
dgondos wrote on 2023-12-16, 09:24:
I found a fork of MBF called MBF_SMN (by Sakitoshi) on github: https://github.com/Sakitoshi/mbf_sigil . The explicit goal seems to be to add support for Sigil (and some other wads) on top of MBF 2.04. I wanted to use MBF on DOS to play Sigil II so I hacked on some patches on top of it to support it - seems to work OK. Is anyone else using this fork or is everyone using a different patch for ep5/ep6 support?
Your PR may need several corrections:
- D_MAIN.C / D_DoomMain(): modifiedgame++ should be moved inside the loop, after D_AddFile() call. Otherwise it will increment just once when several WAD files were specified after a single -file key
- G_GAME.C: a global array pars[6][10] should be extended to [7][10] and {0} initializing value for a 6th episode added, otherwise loading Sigil II will result in writing behind its boundary in deh_procPars() and corrupt the memory; this may cause a Segmentation Violation on MBF start
- G_GAME.C / G_InitNew() contains a hardcoded range check: if (episode > 5) episode = 5;
It could make sense to replace it with the following: if (episode > 4 + modifiedgame) episode = 4 + modifiedgame;
Thanks! Agreed with all of those points, fixed them in the PR now 😀
dr_st wrote on 2023-12-18, 09:44:
I turned modifiedgame into an integer rather than a boolean to count the amount of PWADs requested by the user. It's used later to handle the restriction on max number of episodes. This still isn't great because we don't do any checks on whether those PWADs are Sigil & Sigil II - so it basically means that we assume that if you load two PWADs then one will be EP5 and the other EP6. Previously we assumed that any new PWAD will be EP5. If I understand correctly the way they solved this in prboom is they just set the max number of episodes to a high number (7) and they no longer check if the user requests an episode which is between 4 and 7 but doesn't map to a real episode in a loaded wad.
This in general sounds like a terrible hack.
I wonder if there is a way to dynamically check, early enough in the game loop, whether lumps E5M1 and/or E6M1 have been loaded (from whatever WAD). And if so, present the proper prompts, and configure whatever else is required.
I had a look at this and yes, that should work. I have a prototype patch which checks whether lumps named "E5M1" or "E6M1" exist and modifies the "gamemission" (official mission packs which are packaged as pwads like master levels, no rest for the living, etc.) to set it to Sigil/Sigil 2. We can also use something less specific like a global "maxepisode" flag or something like that which will support other PWADs as well that use episodes 5+. Basically, this `modifiedgame` flag is only used for these purposes:
- Check for IWAD integrity in the case of registered doom => we don't need to modify this check
- For retail (ultimate) doom cap the episode number to some max expected value => This is done in G_InitNew, which is called way after setting up the PWADs. So this can be changed to use our new flags.
- In WI_drawStats() to check whether or not to display par times => No problem here to use our flags.
Additionally I checked as well and couldn't find any code between the beginning of D_DoomMain and where the PWADs are read that would need to be changed for Sigil/Sigil 2 (with the possible exception of the game title and version on the DOS text-mode loading screen, which I guess Romero would prefer to keep as "DOOM Registered Startup" / "The Ultimate DOOM Startup" for the Sigils), so I think it would be possible to use the flags throughout the codebase to replace the specific checks for episode numbers.
Btw, do you guys know if anyone looked at implementing MAPINFO support? It might not be that complex to partially support just the top most frequently used properties from the format. Might make a lot more PWADs playable in MBF.
dr_st wrote on 2023-12-18, 09:44:
How does Ultimate Doom EXE determine whether it's running with Ultimate Doom or Registered Doom? I suppose it must be some godawful harcoded test on the WAD size.
This fork by Sakitoshi does use WAD size / filename / checksum to determine which "gamemission" is loaded, but for differentiating between IWADs (registered/ultimate doom/TNT/plutonia/etc.) it's using a combination of filename checks and pre-scanning the IWAD header for map names. The latter looks like code from the original MBF from '98.