Finishing up PR 481

[selliott4]

Since PR 481 has been open for over a year I thought I'd see if there's anything I can do to finish it up.

 

PR 481 is about making sure that he nap name for the WAD files in the "levels" directory match the file names. My interest in this has to do with the fact that wad2image that I wrote, which has been integrated into Freedoom's "Makefile", generates images based on the map name, not the filename. This means that the output is confusing when the map name is incorrect. wad2image works this way since it can work with WAD files that contain multiple levels.

 

PR 481 has two parts, the fix, which literally alters only a few bytes per effected WAD file, and a shell script that can report on the problem in the future including what the current incorrect names are, and what they should be changed to.

 

For the script I thought it might be nice to add a basic test scaffolding. In this case a new target "test" depends on a "test-map-names" target which runs "tests/test-map-names". That way if someone wants to add a test in the future there's a pattern to follow. But if you don't like it I can change it. I can move the script to the "scripts" directory with a "test" prefix. I can get rid of the build targets, or whatever you want.

 

PR 545 adds instructions on how to do enable binary diffing in git. If those instructions are followed here's what it looks like for one of the files that PR 481 changed:

diff --git a/levels/map18.wad b/levels/map18.wad
index b2ea036..1e9d1b6 100644
--- a/levels/map18.wad
+++ b/levels/map18.wad
@@ -15179,7 +15179,7 @@
 0003b4e0  00 b5 04 f8 04 02 05 0c  05 ff ff 00 00 b5 04 f8  |................|
 0003b4f0  04 ff ff 00 00 b5 04 f8  04 f9 04 ff ff 00 00 b1  |................|
 0003b500  01 b5 04 ff ff 0c 00 00  00 00 00 00 00 4d 41 50  |.............MAP|
-0003b510  30 36 00 00 00 0c 00 00  00 34 17 00 00 54 48 49  |06.......4...THI|
+0003b510  31 38 00 00 00 0c 00 00  00 34 17 00 00 54 48 49  |18.......4...THI|
 0003b520  4e 47 53 00 00 40 17 00  00 d8 6b 00 00 4c 49 4e  |NGS..@....k..LIN|
 0003b530  45 44 45 46 53 18 83 00  00 d0 6a 01 00 53 49 44  |EDEFS.....j..SID|
 0003b540  45 44 45 46 53 e8 ed 01  00 f8 1e 00 00 56 45 52  |EDEFS........VER|

Since I can understand that a relative unknown (me) changing a bunch of WAD files might be concerning I thought PR 545 might be helpful in this case, and possibly future cases, to make it easier to see that there are no extraneous changes.

[Ferk]

I wonder if it's worth the maintenance burden of keeping the names of the lumps always matching their position whenever they are rearranged.

Another way to go about it would be having them all with lumpname MAP01 (or E1M1 for maps meant for Doom 1) independently of their position, since it seems what matters for the build script is the file name.

[selliott4]

1 hour ago, Ferk said:

I wonder if it's worth the maintenance burden of keeping the names of the lumps always matching their position whenever they are rearranged.

 

That's a good question. I thought (hoped?) that things are settling down and that rearrangements or replacements are getting to be less common, but you know better than me. In any case I don't mind checking in again in a few months and fixing it again.

 

1 hour ago, Ferk said:

Another way to go about it would be having them all with lumpname MAP01 (or E1M1 for maps meant for Doom 1) independently of their position, since it seems what matters for the build script is the file name.

 

True, that could be done, but it would make wad2image output even more confusing. It's also possible I should update wad2image to generate image files based on the filename passed to it.

 

In any case, one question is if the map names have any value for anything else such as build error messages, making things less confusing for people editing the files, people consuming Freedoom resources, or whatever. I don't know the answer though.

 

The map name is the name of the first lump, which has a zero byte value. I assume that lump is required, but what if it could be stripped out of the WAD files in the "levels" directory combined with something being passed to duetex, possibly with a duetex modification, telling it what map name each file was? That may be getting more ambitious than we want to talk about though.

 

And finally, a script could be written that automates fixing the map names. Since the map name is just the first lump name it could be done with a Python script that decodes reads the WAD file directly without any dependencies, and then writes the correct name. It would have some regexs to map from Freedoom filename, to Doom map name. I don't think it would be too hard. I could possibly write such a script.

[selliott4]

After thinking about it a bit more I think they way to go is to have a Python script, sort of like the one I just proposed, that can both fix the map names *and* test that if they are correct. So something like "scripts/fix-map-names" since I think there is a desire to keep scripts in "scripts". By default fix-map-names would iterate through the files in "levels" and print out a table of any incorrect WAD files, the map name it had, and the correct map name it was changed to. It could be run after replacing or reordering maps.

 

To test if the maps names are correct the new "test-map-names" build target would call something like "fix-map-names -t" where the "-t" option would test, which would effectively make it a dry run.

 

This way the concept of having a basic test scaffolding run by "make test" is maintained where people could add "test-*" build targets, anyone could easily fix or test the map names at any time, and redundancy between testing and fixing scripts would be avoided (my proposed fix-map-names script would obsolete the current test-map-names in my PR).

 

How does that sound? I would probably submit such a script some evening this week, or the weekend.

[selliott4]

I just submitted PR 547 with what I proposed in my previous comment. I've also closed out PR 481 since PR 547 replaces it.

[Doctor Nick]

Thanks selliott! I'll take a closer look this week.