UMAPINFO bossaction crossable linedef types don't work

[JadingTsunami]

UMAPINFO bossaction is supposed to support crossable and switchable linedef types, but presently crossable types are broken.

This is because the code only attempts to cross the dummy linedef if P_UseSpecialLine returns false. However, for valid linedef types, P_UseSpecialLine returns true.

p_enemy.c:2263
              // use special semantics for line activation to block problem types.
              if (!P_UseSpecialLine(mo, &junk, 0, true))
                  P_CrossSpecialLine(&junk, 0, mo, true);

I see two obvious potential solutions. There may be more. I thought I'd consult others to see which is preferred.

1. The default case in P_UseSpecialLine could return false if a valid usable linedef type is not found.
2. Always try both using and crossing the dummy linedef.

I'd be happy to submit a PR to fix this once a solution is settled on.

[fabiangreffrath]

1. The default case in P_UseSpecialLine could return false if a valid usable linedef type is not found.

This could break unrelated code paths, e.g. in P_Move().

2. Always try both using and crossing the dummy linedef.

I guess this is the safer solution and also what is actually meant to happen. I wonder, though, that the same non-functional calling convention is already present in the A_LineEffect() code pointer (where this apparently has been taken from) and nobody bothered so far.

[jeffdoggett]

I used option 1 and ensured that every single action returned the correct true/false depending on whether it was actually performed. Some caution is needed however, with this, as for instance it can break Hacx Map 17, which expects an always true result. There's some switches behind shootable covers in this level.

[kraflab]

I'm with Fabian on that one. You've got to be suuuuuuuure there is no effect on any of the code downstream if you want to change the return values.

[JadingTsunami]

I confirmed that A_LineEffect is broken in the same way.

I can fix both with a single PR if that's preferred, using option 2 above.

Please confirm that's the desired direction and I'll submit a simple fix for both.

[fabiangreffrath]

No, please don't "fix" A_LineEffect(), we need to preserve its exact behavior for demo compatibility. All we can do is fix the UMAPINFO implementation that this very port introduced. A comment would be nice, though.

[JadingTsunami]

Hmm, but aren't we breaking demo compatibility in the same way by changing UMAPINFO behavior? Just for newer demos created with this port, of course. Whether or not there are any WADs/demos with broken DeHackEd/BEX calls to A_LineEffect out there, I don't know, but I don't know for UMAPINFO either.

Lastly, before I submit the PR, does anyone know the intent of the statement below? It was added as part of the UMAPINFO code. By removing the "if" are we inadvertently allowing these problem types to reach the P_CrossSpecialLine and break something?

p_enemy.c:2263 // use special semantics for line activation to block problem types.

[kraflab]

I think there are yet zero umapinfo demos on dsda, so there's probably not a demo compatibility issue at this stage to change umapinfo-specific behaviour - although if someone in the future is using an old version of prboom+ they can get different results - that's a good point...

Given this is a bug fix, the onus would be on future versions to implement another "unfix" compatibility flag, if it comes to that.

[jeffdoggett]

I very much doubt that there are any maps in existance that would use P_CrossSpecialLine() since it doesn't work. I would expect the map designer to change it to something that does work before it's even released.

[JadingTsunami]

I suppose the same argument could be made for the broken Crusher generic line specials.

Although this particular UMAPINFO problem is a trivial issue, I wonder if it is preferable to draw a clear boundary on the port's promises in terms of demo compatibility, both with existing mbf-complevel demos and its own prior revisions.

If I understand the conversation, it is something like:

1. This port must be functional with all demos recorded with PrBoom versions prior to this port.
2. This port may be functional with demos recorded with older versions of itself.

[kraflab]

This port is able to play back all demos recorded by any past version. There is a long list of special cases in the code to handle temporary "accidents" in past versions that led to bad behaviour that was later fixed. That way, someone who records on an old bugged version even today will produce a demo that can be played back by every version after that. I don't see any reason to abandon that philosophy.

[JadingTsunami]

In that case, we should not fix this bug, because demos recorded with older versions of this port could de-sync.

EDIT: I am making the unspoken assumption that new complevels are forbidden, because I remember hearing that discussed. I don't mean to break open that debate if it's considered "closed."

[fabiangreffrath]

I hope it's clear that we won't change an MBF code pointer. The code has been like that for the past 22 years, and this port is not here to fix historic bugs, it is here to preserve them.

Regarding UMAPINFO, though, the ability to record proper demos has been added some four weeks ago. And now is the unique chance yo get the reference implementation right before any serious harm is done. I'd say at this point it is safe to assume that no demo exists yet that makes use of this specific UMAPINFO feature. Or not?

[kraflab]

I think that's a safe assumption

[JadingTsunami]

I don't think it's about existing demos (which almost certainly don't exist), I think it's about the fact that the binaries are out there where demos can be recorded and still have this bug. Now there is a situation where PrBoom-Plus-2.5.18um demos could de-sync on PrBoom-Plus-2.5.19um+ binaries (or whatever the version numbers are).

But, if the consensus is that we're early enough to fix things like this, I don't have a strong opinion on it.

[fabiangreffrath]

Hm, another pov to take into consideration is thst of mappers. Demo compat aside, the UMAPINFO code is in this port for some four years now. Do we know of any maps released or currently under development that rely on the broken boss action behavior?

[JadingTsunami]

The only one I'm aware of that uses BossAction is Fork in the Road, but I don't think it is reliant on the current implementation. There may be more; this is just the one that I know about.

Of course, I am not certain of that. It would require more in-depth analysis.

[fabiangreffrath]

Sigh this is a can of worms.

[JadingTsunami]

Indeed. Perhaps we should encode the PrBoom-Plus-2.5.xx version in the demo, and any demo-breaking fixes are compared against the recording version and applied or not on that basis. This would make any demo playable in any port, but would prevent newer from playing in older.

This would free us to fix bugs like this, A_LIneEffect, and Generic Crushers linedef types.

[kraflab]

There's already a system in place for fixing these kinds of bugs and toggling flags for demo playback, so I don't think there's any need to change how we write demos (and besides, the "version" usually doesn't change much, so we'd need to start incrementing some subversion for every commit like done in zdoom demos, which sounds like overcomplicating a simple thing that's already handled).

Whether there are maps that expect the existing behaviour is another question though.

By the way, is this bug only in prboom+? Are there other ports that have implemented this yet?

[JadingTsunami]

There's already a system in place for fixing these kinds of bugs and toggling flags for demo playback

You are suggesting a new compatibility level, or is it something else I'm not familiar with?

By the way, is this bug only in prboom+? Are there other ports that have implemented this yet?

GZDoom and k8vavoom are the ones I know of implementing UMAPINFO, but it doesn't look like either have this bug.

[kraflab]

It's not a new complevel - there's a series of flags that toggle certain specific behaviour on in case of a demo needing something uncommon. They're settable with separate command line options.

[JadingTsunami]

I see. The same idea as a version number but tied to specific features instead.

I suppose it is an option. Given the lifespan of the port, the un-fixed, demo-breaking bugs should be a known quantity. They could all be fixed in this way.

[JadingTsunami]

I think a decision will need to be made here, and it should set the direction for all demo-breaking bug fixes like this going forward. Unless we are going to leave all current demo-impacting bugs in the port forever. That is a valid option, but I think a limiting one.

To make the right choice I think the goals must be clear.

I see them as:

1. Any demo recorded on an older version must play back correctly on any newer version.
2. Any demo recorded on a newer version should abort when played on an older version if the demo would be incompatible (i.e., there are missing features in the older version required for accurate playback).

Are there more thoughts? The first goal can be achieved with a new compatibility flag, although we are very low on space there (5 bytes remaining by my count). We can move to bitwise flags for the final 5 bytes to buy time.

The second goal is trickier. But I don't see it as absolutely critical, more of a good-to-have item.

[JadingTsunami]

A deafening silence. :)

Alright, I take this as uncertainty, so I'll make a suggestion. Let's fix all the bugs like this (yes, even A_LineEffect) and put in demo compat flags like @kraflab suggested. Use the remaining space bitwise to spread out the 5 bytes we have left.

This will enable mappers to use functions that should have worked since forever (like generalized crushers). We've already broken ground here by adding things like DEHEXTRA.

Reactions?

[kraflab]

My opinion on crushers remains unchanged since that was brought up here https://github.com/coelckers/prboom-plus/issues/57

Whether by accident or intent, those are not part of boom / cl 9, and they don't belong there.

Fixing the umapinfo thing is a umapinfo question, which is easier to discuss. General bugs (especially ones that are decades old potentially) need a strong reason to be "fixed". Most of the way doom works is bugged in one way or another 😅

[JadingTsunami]

@kraflab can you expand on your reasoning a bit? What bad things do you foresee happening? What would you do differently if the answer was "fix it"? I think your concerns are important to capture and I want to make sure I understand them fully.

[kraflab]

When it comes to the crushers, we're talking about adding linetypes that didn't functionally exist in boom. Thus it is a feature request and not a bug fix. That's something that I could see being added into the new header system if we want to add more prboom-specific feature toggles, but I don't think it makes sense to call that a compatibility flag.

[fabiangreffrath]

Sorry, but what 5 bytes left are you guys talking about?

[JadingTsunami]

Sorry, but what 5 bytes left are you guys talking about?

In the demo header, there are a set of compatibility flags to enable/disable potential demo-breaking functionality.

See: src/g_game.c:4016

[kraflab]

Sorry, I'm not talking about the header bytes either 😅

There are commandline toggles for compatibility-related bugfixes.

We can't really edit the header bytes - those have specific meanings tied to a given complevel.

Here's an example of this: https://github.com/coelckers/prboom-plus/issues/164

[JadingTsunami]

We can't really edit the header bytes - those have specific meanings tied to a given complevel.

Hmm, the unused ones are ignored as far as I can see. So not sure why you would say this.

Can you expand on it?

[kraflab]

No meaning is an important meaning. We aren't going to change the legacy demo formats every time someone has a feature idea.

[JadingTsunami]

Hmm, I still don't understand these value statements; why is it important? It won't break anything and improves the mapper and user experience while preserving the past behavior.

What bad thing will happen?

[kraflab]

You record a boom demo on your port, then send me the boom demo and I play it back and it desyncs.

[JadingTsunami]

You record a boom demo on your port, then send me the boom demo and I play it back and it desyncs.

Sure, certainly we should build that into any support we add for potential demo-breaking changes. In fact I said this already earlier in the thread:

1. Any demo recorded on an older version must play back correctly on any newer version.
2. Any demo recorded on a newer version should abort when played on an older version if the demo would be incompatible (i.e., there are missing features in the older version required for accurate playback).

So I think we agree that case must not be allowed to happen.

[Ferk]

I think kraflab is right that it'd be wiser to place any new optional features that affect compatibility into the array of options from the new extensible demo format (recently introduced in https://github.com/coelckers/prboom-plus/pull/107 ), rather than playing around with very old and very stable compatibility flags that are harder to extend and risky to play around with.

Maybe there should be a name for those option strings, to not confuse them with the compatibility flag bits.

[JadingTsunami]

Maybe there should be a name for those option strings, to not confuse them with the compatibility flag bits.

I think this is fine, and don't mind the approach. We could define a new extension block of compatibility options and only play demos if all the flagged options are supported.

[Ferk]

Yes, that's basically what #107 did already. At the moment the only "demo option" (for lack of a better name) currently supported in that block is the "UMAPINFO" string, but it's extensible so more could be added for new options.

When an unknown "demo option" is found, currently an error is printed. Perhaps it should give up on playing the demo.

https://github.com/coelckers/prboom-plus/blob/d840f4258568acbefa1c4608526f2e4469496f8b/prboom2/src/g_game.c#L3774-L3784

Oh, actually in the code it's referred to as "extension string".

[kraflab]

I think an error makes sense. There's nothing to do if you are trying to play a demo that can't be played. It could be improved to say what the extension was though. I think it would be nice to have some metadata in extensions as well, so it could say "This demo requires feature X, which was introduced in version Y of the extended header" for instance.

[fabiangreffrath]

The port shouldn't give up on playing a demo just because it has been recorded with a different port that supports e.g generalized crushers when there isn't any such crusher encountered in the course of the demo, though.

[JadingTsunami]

The port shouldn't give up on playing a demo just because it has been recorded with a different port that supports e.g generalized crushers when there isn't any such crusher encountered in the course of the demo, though.

This sounds like a new complevel to me, then. Or, we have to get really fancy and set the demo flag only if a broken case was encountered and fixed.

[fabiangreffrath]

This sounds like a new complevel to me, then. Or, we have to get really fancy and set the demo flag only if a broken case was encountered and fixed.

I meant to suggest the latter and also to show up how deep the can of worms may become this is going to open.

[Shadow-Hog]

The only one I'm aware of that uses BossAction is Fork in the Road, but I don't think it is reliant on the current implementation. There may be more; this is just the one that I know about.

Of course, I am not certain of that. It would require more in-depth analysis.

As the author of said mod, I figured it might be helpful to chime in: I explicitly stuck with S1 actions because I didn't think W1 actions would work.

There are demos in the mod (DEMO1-4), but they were recorded while UMAPINFO's relationship with demo compatibility was still being hotly debated and I anticipate I'll be rerecording them in future revisions anyway. I don't think fixing the W1/WR-handling code will break them as such (if they're not already broken), but that's something that'll have to be tested I guess.

[JadingTsunami]

Seems like a good time to re-raise this issue, as the new MBF21 complevel is being proposed.

This seems like the ideal opportunity to fix both the A_LineEffect and BossAction/UMAPINFO issues.

[fabiangreffrath]

But we would have to support MBF21 complevel as a whole on order to properly implement this. And this is a rather long way...

[JadingTsunami]

But we would have to support MBF21 complevel as a whole on order to properly implement this. And this is a rather long way...

Sure, but UMAPINFO (and MBF21) is cross-port. I think the PrBoom+ implementation schedule can differ.

I just want to ensure we don't miss this opportunity to get these fixes incorporated given the rarity of new complevels.

[kraflab]

I think the opportunity is already missed as far as code fixes are concerned. UMAPINFO is a "module" rather than a complevel, so you should be able to make changes there - I would recommend setting up something in the umapinfo header area that says what revision of umapinfo is used, and then ports can adapt to the revision (basically a parallel stream of compatibility).

[JadingTsunami]

UMAPINFO is a "module" rather than a complevel, so you should be able to make changes there - I would recommend setting up something in the umapinfo header area that says what revision of umapinfo is used, and then ports can adapt to the revision (basically a parallel stream of compatibility).

We explored that option earlier in this thread, but the conclusion was it would be too disruptive to demo cross-compatibility and playback.

[kraflab]

We explored that option earlier in this thread, but the conclusion was it would be too disruptive to demo cross-compatibility and playback.

It's not an issue in the extended header of version 255 - adding stuff there is the whole reason it exists. The sooner we start storing the umapinfo version in the header the better imo.

[JadingTsunami]

It's not an issue in the extended header of version 255 - adding stuff there is the whole reason it exists. The sooner we start storing the umapinfo version in the header the better imo.

I generally like the idea. I think the point (as I understood it) was that the established norm is to ignore unexpected extended header fields, so there is no way to stop a demo from de-syncing if it is played back on an incompatible version.