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