coelckers / prboom-plus

This is a cleaned up copy of the PrBoom+ SVN repository as a courtesy for those interested in forking that port
287 stars 115 forks source link

UMAPINFO demos compatibility #337

Open rfomin opened 3 years ago

rfomin commented 3 years ago

There are at least two PWADs (Fork in the Road and DBP25: Dead, But Dreaming) that have embedded UMAPINFO demos recorded with released PrBoom+UM 2.5.1.7 version. So, in PrBoom+ tradition, there should be backward compatibility?

What I really hope to achieve with this issue is a discussion about UMAPINFO demo extension.

  1. Why we need "PR+UM" signature and why other signatures are not supported (e.g. "WOOF+UM" or "CRISPY+UM")?
  2. I suggest removing the map name recording as there is no support in the spec.
  3. I suggest removing all extensions, all we need to do is warn about recording a UMAPINFO demo for WAD without built-in UMAPINFO and let users figure out their UMAPINFO situation on their own. I think new actual complevels are the better way to add demo incompatible features/bugfixes like MBF21.
kraflab commented 3 years ago
  1. The value itself isn't important, but it's a safeguard against other ports that might also take version 255. It's the indicator that the extension header has the format we expect, in the same way the mbf signature doesn't have anything to do with the port a demo is recorded on, just to indicate the format - and if another port uses a different format, they can use a different signature with the same version.

  2. I don't really disagree with this one. When there's support, we should do it, but if not then not.

  3. Here's the motivation for the extension "wrapper" (which I think better indicates its purpose). Let's look at this through the lens of something that didn't use the extension header. Crispy doom has added support for some dehacked features that don't exist in prboom+. If you have a dehacked that uses those fields and record a demo in crispy, it will be a vanilla demo but it uses non-vanilla features. Then if you play that in prboom+ it will desync. Instead of that, you could add an extension to the header, like "CRISPY-DEHACKED" that indicates the demo requires a nonstandard feature. Then the demo playback will abort in prboom+ (and any other port not supporting that extension). This approach allows you to extend a compatibility level with a new feature without introducing a new one. More importantly, you can pick and choose the features.

An alternative would be adding tons of complevels, which isn't really feasible. Another alternative is restricting all new features to new complevels - that's an option but then for instance crispy can't support any new features without implementing everything up to the new level. With the extension header, nothing stops crispy from just implementing a given extension and allowing maps that are otherwise vanilla to use that extension without digging into any compatibility changes.

Yet another alternative is to basically say "the map doesn't work in pr+ anyway - trying to play a demo for a map that doesn't work in your port makes no sense and we don't need to account for it". That's to say "it's fine that it desyncs, we don't need to change the compatibility level or indicate the feature exists in the demo itself". I like that approach - but it does have a problem. What happens if you want to change how that new feature works, in a breaking way? If the information about that feature isn't inherent to the complevel, then how can you switch between old and new behaviour? Personally, I would prefer to take this approach and require a line in umapinfo saying what spec version it uses - then we can toggle behaviour based on that version. We can assume any umapinfo without a spec version listed is the last version before adding this requirement.

kraflab commented 3 years ago

In general I kind of feel like the idea behind the extension header is good, but in practice it doesn't really provide any real advantage, while simultaneously promoting a spaghetti feature world where every port supports only pieces of the standard. Maybe it should be nipped in the bud before too many demos are recorded - and we can replace the header handling in pr+ with something that detects this header and strips it off :shrug:

rfomin commented 3 years ago

Maybe it should be nipped in the bud before too many demos are recorded - and we can replace the header handling in pr+ with something that detects this header and strips it off.

I agree with that. I think we should be more realistic - I want a world with dozens of demo-compatible ports with hundreds of features, but we actually have PrBoom+, DSDA-Doom, Crispy/Chocolate/Woof, and possibly Eternity. The other ports don't seem to be interested in demo compatibility.

kraflab commented 3 years ago

https://github.com/kraflab/dsda-doom/commit/5c8fedf1405e11ceb5be2e30ed9b168780cf28d7