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

Add "GAMEINFO" IWAD field support #603

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

Conversation

andrikpowell
Copy link
Contributor

@andrikpowell andrikpowell commented Feb 6, 2025

So this is going to be a fun one...

This solution is pretty jank and probably not the best implementation... but it does work!

This is similar to the GAMEINFO lump that GZDoom uses that can specify an IWAD to load via a PWAD lump.

Essentially to search the PWADs for an "IWAD" lump, I needed to cache the lumps before choosing the IWAD. So what ends up happening is that I cache it once (as a sort of quick cache), read the IWAD lump and then clear the cache for the "Real" lump cache.

Currently the main thing I don't like is, I haven't been able to combine the zip functions together as one with the variable "MainLumpCache".

If you're wondering what this is useful for, it's so I can drag-and-drop a plutonia based wad on dsda-doom, and it can load plutonia.wad if it finds it in the directory. I have a clause that defaults to normal behaviour if the wad specified in the "IWAD" lump isn't found.

@rfomin
Copy link
Collaborator

rfomin commented Feb 7, 2025

This is similar to the GAMEINFO lump that GZDoom uses that can specify an IWAD to load via a PWAD lump.

Why not support GAMEINFO lump or ID24 GAMECONF? We don't have to support all fields.

I don't think we should add another lump, many WADs already contain GAMEINFO/GAMECONF.

@andrikpowell
Copy link
Contributor Author

This is similar to the GAMEINFO lump that GZDoom uses that can specify an IWAD to load via a PWAD lump.

Why not support GAMEINFO lump or ID24 GAMECONF? We don't have to support all fields.

I don't think we should add another lump, many WADs already contain GAMEINFO/GAMECONF.

I agree. The IWAD lump was more an example about how it would work. This PR was mostly for the functionality of choosing an IWAD based on a PWAD lump.

Personally I'd rather support GAMEINFO. It's simpler and it could just look for the line iwad = "plutonia.wad" (example wad). Although I can't say I'm that knowledgeable when it comes to parsing text lumps in general.

@rfomin
Copy link
Collaborator

rfomin commented Feb 7, 2025

Personally I'd rather support GAMEINFO. It's simpler and it could just look for the line iwad = "plutonia.wad" (example wad). Although I can't say I'm that knowledgeable when it comes to parsing text lumps in general.

Basically, DSDA-Doom already has a ZDoom scanner, see src/scanner.h. It used to parse *MAPINFO and UDMF.

@andrikpowell andrikpowell changed the title Add "IWAD" lump support Add "GAMEINFO" IWAD field support Feb 7, 2025
@andrikpowell
Copy link
Contributor Author

Personally I'd rather support GAMEINFO. It's simpler and it could just look for the line iwad = "plutonia.wad" (example wad). Although I can't say I'm that knowledgeable when it comes to parsing text lumps in general.

Basically, DSDA-Doom already has a ZDoom scanner, see src/scanner.h. It used to parse *MAPINFO and UDMF.

Thanks! This took a little longer because I was looking at UMAPINFO, UDMF .cpp files... I really should've been looking at a more simple example like ambient.cpp instead :P

Your code really helped... Although I'd like to get better at understand C++ and learn how to get C and C++ to work together :)

Now that I see the "GAMEINFO" support in action, I really like it!

@andrikpowell andrikpowell force-pushed the iwad-lump branch 5 times, most recently from 64611ac to 29de070 Compare February 7, 2025 09:42
@Pedro-Beirao
Copy link
Collaborator

Pedro-Beirao commented Feb 7, 2025

I like the idea of supporting GAMEINFO. But lets also support the LOAD field yeah?

And woof should probably do the same

rfomin

This comment was marked as outdated.

@rfomin
Copy link
Collaborator

rfomin commented Feb 7, 2025

But lets also support the LOAD field yeah?

Is it practical? I have never seen a PWAD that uses it.

And woof should probably do the same

Yes, agreed.

@Pedro-Beirao
Copy link
Collaborator

Is it practical? I have never seen a PWAD that uses it.

I also dont know of any PWAD that uses it, but I can imagine reasons to want it.
For example a vanilla wad that has widescreen assets distributed in a separate pwad

@andrikpowell
Copy link
Contributor Author

Is it practical? I have never seen a PWAD that uses it.

I also dont know of any PWAD that uses it, but I can imagine reasons to want it. For example a vanilla wad that has widescreen assets distributed in a separate pwad

I'm not sure this works the way you think it works exactly. According to the ZDoom wiki, the LOAD field loads certain wad files BEFORE the wad (usually iwad) that I includes the GAMEINFO lump.

In the example of a wide-screen asset wad, the LOAD lump wouldn't allow for wide-screen assets to be used, because they would be overwritten by the iwad.

@Pedro-Beirao
Copy link
Collaborator

My bad then, I didnt read the "before" in the wiki. Well that sucks... what uses does loading before even have?

The simple "IWAD" lump you suggested before is looking cleaner now tbh. With the drawback that no current wads support this lump yet, unlike GAMEINFO.

I think its worth thinking more about this, as supporting a lump just for one field seems wrong. Wouldve prefered if GAMEINFO had fields for iwad, pwads and complvl.
The name is also confusing, as it is part of Z***INFO lumps. We should either get a new UGAMEINFO with the fields we need, or just support the "loose" lumps of COMPLVL and IWAD.
(Also mentioning your suggested MODINFO/UMODINFO lump on the discord)

@andrikpowell
Copy link
Contributor Author

andrikpowell commented Feb 7, 2025

My bad then, I didnt read the "before" in the wiki. Well that sucks... what uses does loading before even have?

I think it's more focused around zdoom projects separating resources into different pk3s (like graphics, sounds, etc)... Perhaps even making some resources optional, which the iwad would then check if those existed.

The simple "IWAD" lump you suggested before is looking cleaner now tbh. With the drawback that no current wads support this lump yet, unlike GAMEINFO.

I think its worth thinking more about this, as supporting a lump just for one field seems wrong. Wouldve prefered if GAMEINFO had fields for iwad, pwads and complvl. The name is also confusing, as it is part of Z***INFO lumps. We should either get a new UGAMEINFO with the fields we need, or just support the "loose" lumps of COMPLVL and IWAD. (Also mentioning your suggested MODINFO/UMODINFO lump on the discord)

I actually disagree here. The main reason I took the "IWAD" lump approach before is because I thought it'd be too difficult to parse the "GAMEINFO" lump. If I knew it was as easy as this, I would've gone with "GAMEINFO" originally.

There's a couple points I want to point out here. GAMEINFO is a GZDoom lump.. so it's not that weird to only parse some of the lump. In fact, it doesn't make any sense for it to parse complevel, since that isn't even zdoom related.

I actually am highly against having a new lump for multiple reasons.

  1. GAMEINFO is actually a very common lump in Plutonia/TNT/Doom1 wads. For example, I used Plerbtonia for testing, and it already had the lump in the wad. I was surprised to learn that my own RUST doom1 wad also had a GAMEINFO with and iwad specification there. It's not uncommon nowadays for modders to also target zdoom ports, and thus most wads for specific IWADs have GAMEINFO nowadays.
  2. I think the main thing you are overlooking is that GAMEINFO has been around for over a decade, and modders have been using it to define exact IWADs. This basically means that adding this feature, we would support lots of legacy wads. The problem with adding a new wad is that modders would have to add yet another lump to their wad. Plus it really makes no sense to create a new lump when many doom wads already have a lump that does the same purpose.
  3. I'm not sure what you mean by the "IWAD" lump approach being cleaner... Both approaches use around the same amount of code. Only difference is that "GAMEINFO" is using a parser similar to umapinfo or udmf. When we parse those lumps, we already don't parse things that aren't applicable to dsda-doom (MAPINFO being a big example).

@rfomin
Copy link
Collaborator

rfomin commented Feb 8, 2025

I think its worth thinking more about this, as supporting a lump just for one field seems wrong. Wouldve prefered if GAMEINFO had fields for iwad, pwads and complvl.

We can add new fields to GAMEINFO, GZDoom would ignore them.

GAMECONF has pwads field, executable for complevel (although I disagree with it) and interesting wadtranslation (converting a WAD's graphics from its original PLAYPAL to a new PLAYPAL included by a PWAD). GAMECONF has also been added to many WADs for compatibility with the KEX port, and we can also add new fields to it if we want.

@andrikpowell
Copy link
Contributor Author

We can add new fields to GAMEINFO, GZDoom would ignore them.

We could do this... but honestly it feels kinda useless and undermines the whole point of the COMPLVL lump. The main point of the COMPLVL lump was that it was supported by a majority of source ports. So most wads would keep this lump regardless, unless you are expecting other source ports to magically support GAMEINFO as well.

The main appeal for me was that old wads already had GAMEINFO to pick the IWAD from.

GAMECONF has pwads field, executable for complevel (although I disagree with it) and interesting wadtranslation (converting a WAD's graphics from its original PLAYPAL to a new PLAYPAL included by a PWAD). GAMECONF has also been added to many WADs for compatibility with the KEX port, and we can also add new fields to it if we want.

I realise this is probably just a "me" thing, but I very much dislike GAMECONF. It would be much more code to support it, due to it being in JSON format. I recently asked my discord server if they preferred GAMEINFO or GAMECONF, and they said GAMEINFO due to it being easier to write from scratch.

I think it may possibly be worth supporting at some point... maybe... Although me and many other modders are quite unhappy about how ID24 is formatted and how not user-friendly it is. So I'll admit I'm a bit biased against GAMECONF, but if we eventually support GAMECONF, we definitely should support GAMEINFO.

@rfomin
Copy link
Collaborator

rfomin commented Feb 8, 2025

We could do this... but honestly it feels kinda useless and undermines the whole point of the COMPLVL lump.

But we can add an ALTLOAD field for additional WADs that will work as we want.

@andrikpowell
Copy link
Contributor Author

andrikpowell commented Feb 8, 2025

Figured I'd mention this is ready for an other review.

Now zip files have been fixed, as well GAMEINFO reading via autoload.

This PR should be fully complete now, in regards to the functionality of allowing PWADs to define an IWAD and GAMEINFO support.

IMO any other lumps defining IWADs should be a separate PR.

Copy link
Collaborator

@rfomin rfomin left a comment

Choose a reason for hiding this comment

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

Seems to work fine, thank you.

Z_Free(iwadlump);
}

G_ReloadDefaults(); // killough 3/4/98: set defaults just loaded.
Copy link
Collaborator

@rfomin rfomin Feb 9, 2025

Choose a reason for hiding this comment

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

Why did you move G_ReloadDefault() here? HandlePlayback() may be affected by this. I'd rather not move G_ReloadDefault() if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason why I moved G_ReloadDefault() is because the game crashes if it doesn't come after the second W_Init() call.

I haven't figured out the reason why. I'd prefer it to be above Handleplayback() like before, but currently it's there to avoid the crash.

Copy link
Contributor Author

@andrikpowell andrikpowell Feb 9, 2025

Choose a reason for hiding this comment

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

Ah I figured it out! See 37979ea.

The crash was caused because lumpnum and lumpinfo weren't being cleared... Which is a little weird because I know at some point I had that code there.

Anyway, G_ReloadDefaults() should be in the same place as it was before now, and I moved the iwadlump lprintf to happen a bit later so that it shows just before when the "Detected COMPLVL" text shows.

@andrikpowell andrikpowell force-pushed the iwad-lump branch 2 times, most recently from 3d956dd to d4ae017 Compare February 9, 2025 08:16
Copy link
Collaborator

@rfomin rfomin left a comment

Choose a reason for hiding this comment

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

LGTM

@andrikpowell
Copy link
Contributor Author

Figured I'd mention that I added the "Doom 1 lump detection" code into this PR. Hopefully that's fine...

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.

3 participants