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 114 forks source link

Blockmap bug #187

Open felipeforte opened 3 years ago

felipeforte commented 3 years ago

Considering it was never intended by the authors of the game, the blockmap bug sometimes causes an unexpected turn of events in gameplay. The way Doom originally calculated the blockmap and its limitations are addressed in this video, and in a more technical aspect, in this article.

My argument in support of finally fixing the blockmap bug is that above all else, it was a product of oversight in the part of its developers, and in a manner, a product of the limitations of the machines of their era, which required very conservative computational power. To fix this bug, however, we wouldn't require greater computational power than what is currently possible for modern computers.

maxmanium commented 3 years ago

I wouldn't be against this, but it would surely require a new complevel, right? If we're going to do that there's a lot more to be discussed than just the blockmap bug.

felipeforte commented 3 years ago

I wouldn't be against this, but it would surely require a new complevel, right? If we're going to do that there's a lot more to be discussed than just the blockmap bug.

Perhaps, but at least it's an important step that this is being taken into account at some point in development.

kraflab commented 3 years ago

I don't see much motivation for this. That's a core mechanical element of doom. Are we also going to replace the physics engine with zdoom's? Doom is full of these kinds of "errors" and they define it. I'm really continuously struggling to see the drive for all the recent requests to move prboom+ towards zdoom. Zdoom already exists, in a final / stable release, ready for action.

felipeforte commented 3 years ago

Are we also going to replace the physics engine with zdoom's?

What? Who said that? PrBoom+ has the best physics for playing Doom, I never play ZDoom. I only made the suggestion to fix a bug, as in, something considered not intended, or a side-effect of a code too simplified for accurate hitscans.

PrBoom+ already does something similar, by removing the limit for lost souls spawned by Pain Elementals. This was a positive change too, in my opinion, because it makes Pain Elementals even more dangerous, especially with fast monsters.

I play Doom since I was three years old, and now that I am 23 and still playing it to this day, I think I'm fit to say what needs improvement :joy:

The fixing of the blockmap bug could be made to make the game even more enjoyable and playable, decades after it was released. In my opinion, the blockmap bug is not something worth keeping, since it is, in the end, very frustrating to deal with. If you shoot in a specific direction, you are expected to hit in that specific direction, not to have your swarm of Super Shotgun bullets somehow disappear from thin air.

Don't get me wrong, I dislike ZDoom. I really don't like how it plays. But we should bash ZDoom for what it gets wrong, not for what it gets right. I don't care if ZDoom has done it, this is an improvement, even if no other source port hadn't thought of it.

kraflab commented 3 years ago

Is this performance art?

JadingTsunami commented 3 years ago

I have to agree that this would so fundamentally change the game's mechanics that it would go beyond the desired scope of the port.

It is a bug, no question. But this bug is part of the core mechanics of the game at this point. Its removal would change end user gameplay too much to consider fixing at this stage.

I recommend no fix.

Coincident commented 3 years ago

Fixing the blockmap bug would break backward compatibility of demo playback/recording. That is a NO-GO for sourceports such as PrBoom+ that aim towards capturing the legacy doom engine function, which is necessary for both playing back old demos recorded with the original DooM.exe, but also to have a consistent / level playing field for competition (deathmatch, speedrunning, etc).

For the majority of doom players who don't partake in competition, don't play back old demos, and just want a smooth and bug-free casual doom playing experience, ZDoom is the best candidate. There's nothing to dislike about ZDoom in that department; it's highly customizable, and the majority of those bugs have been fixed.

JadingTsunami commented 3 years ago

Fixing the blockmap bug would break backward compatibility of demo playback/recording.

If this bug were fixed, I would expect a new complevel to be created, which would resolve this concern.

felipeforte commented 3 years ago

Fixing the blockmap bug would break backward compatibility of demo playback/recording.

You're right, I didn't think of that initially. Perhaps a complevel could address this?

fabiangreffrath commented 3 years ago

Don't get me wrong, I dislike ZDoom. I really don't like how it plays. But we should bash ZDoom for what it gets wrong, not for what it gets right. I don't care if ZDoom has done it, this is an improvement, even if no other source port hadn't thought of it.

Maybe ZDoom feels so wrong to you because it has implemented all these little "improvements" that you guys keep suggesting. Just a thought...

El-Juancho commented 3 years ago

what if this gets added to complevel -1 and you can turn it off/on in the compatibility options?

felipeforte commented 2 years ago

While I understand that for historical reasons (e.g.: demo playback) this bug will be preserved, prboom-plus is the only source port that is efficient enough to play under the conditions of my hardware. I have followed an advice put here to use GZDoom for the purpose of playing without this unintended feature. However, not only GZDoom is heavy, they have deviated too much from the original design of the game to the point it really plays different. I argue that the blockmap bug is also a deviation from the original design as well, since it was never intended and in most cases burdens the player by no fault of their skill.

The complevel solution would preserve both tendencies, one advocating to maintain the bug for historical and competitive reasons, and the other advocating for the fixing of the bug for other purposes (such as content creators who play Doom casually, live play, etc.)

SirBofu commented 2 years ago

This should be possible without a new complevel and without causing demo desyncs. You would just have to make it a toggleable option ("Use Improved Hit Detection," initialized to No) and tweak the P_BlockThingsIterator (International Doom has already done this) and add some conditions that when the setting is disabled, when playing/recording demos, or when not playing singleplayer, it skips the additional checks. I took the P_BlockThingsIterator from International Doom and modified it considerably, including skipping the additional checks based on multiplayer/demo status, and added the setting (rather clumsily) to the Setup menu.

I was able to do this on my own cloned copy of the repo. I think there is definitely interest in this being an option that can be enabled regardless of complevel.

I created a test map that is intentionally built around the BLOCKMAP. When the map loads, the player will be facing a Zombieman, who is facing away from them. If the player fires right away, the Zombieman will be missed. I even recorded a cl2 demo of the player firing several shots at the Zombieman (pausing between each shot so that they would be perfectly accurate).

I then load the same map in my own local branch with these changes made. At first, the option is off. I load the map, fire in the same manner, and every shot misses. I then enable the improved hit detection setting, fire, and the Zombieman is struck.

The setting is now on. I exit and then restart, loading the demo I recorded; the player fires at the Zombieman and misses, exactly as I had when I recorded the demo. And by disabling the option, the legacy BLOCKMAP behavior is still used, allowing players to choose whether or not they use the improved hit detection.

This required changes to p_maputil.c, p_map.h (PIT_RadiusAttack and PIT_SectorChange need to get moved there if you want them to not throw errors when you use the International Doom code, I believe), m_menu.c (to add the physical option), m_misc.c (to make sure that M_LookupDefault was happy), and doomstat.h.

JadingTsunami commented 2 years ago

The proposal introduces a gameplay modifier that is incompatible with demo recording.

Such features are the purpose of complevels, so this feature still sounds like a new complevel to me.

Perhaps others have feedback as well.

maxmanium commented 2 years ago

The proposal introduces a gameplay modifier that is incompatible with demo recording.

Such features are the purpose of complevels, so this feature still sounds like a new complevel to me.

Perhaps others have feedback as well.

Isn't that the point? This type of feature would never be compatible with demo recording to begin with. I think the proposal is just meant to be a gameplay modifier that is disabled during demos, like vertical aiming.

JadingTsunami commented 2 years ago

Isn't that the point? This type of feature would never be compatible with demo recording to begin with. I think the proposal is just meant to be a gameplay modifier that is disabled during demos, like vertical aiming.

Right, but with a new complevel, this could be made demo-compatible. The only reason I see not to tie it to one would be to avoid introducing a new complevel, which seems "wrong" to me.

maxmanium commented 2 years ago

Right, but with a new complevel, this could be made demo-compatible. The only reason I see not to tie it to one would be to avoid introducing a new complevel, which seems "wrong" to me.

Because that would open a new can of worms -- complevels are there in the first place, aside from correct demo playback, to ensure that maps function as intended by the author, based on the port and\or featureset used. Would we have to introduce a whole new set of complevels (say just the big ones like 2,3,4,9, and 11) just for this feature? That seems like it would really muddy up the codebase and just become really confusing.

You could just make it an offshoot of something like MBF21, but then that doesn't guarantee that older maps work properly since they won't have an OPTIONS lump included. Then you run into the issue of other port authors (mainly kraflab but not exclusively) adapting and supporting this new complevel. Seems better to just make it a gameplay option when not recording or playing back demos.

SirBofu commented 2 years ago

I don't think it'd be a bad idea to have a new complevel with this as a standard, but that's also probably a much more complicated solution than what people are actually wanting: a gameplay fix.

There are already plenty of options that live outside of complevels. Zero tag fixes, players walking under hanging bodies, intercept overflows, freelook - none of these have anything to do with complevel.

Keeping it separate from the complevel allows people to play maps designed for any complevel with the blockmap fixed and not break other compatibility options. It's a common request for people to want to play with the bug fixed; I don't think it's common for them to want it fixed specifically for demos.

kraflab commented 2 years ago

Gameplay changes like this aren't really in scope for this port - especially with it in maintenance mode. If you're looking for a port that supports lots of vanilla behaviour but also has gameplay tweaks, the aforementioned international doom is a good choice. I think nugget doom is a similar variant for woof. I'm not sure if it has this feature, but you could probably ask the dev - I imagine they would be more open to adding it. (Or maybe woof would be interested in adding it directly, you could ask there as well.)

JadingTsunami commented 2 years ago

There are already plenty of options that live outside of complevels. Zero tag fixes, players walking under hanging bodies, intercept overflows, freelook - none of these have anything to do with complevel.

Hmm, ok. I think we should consider merging this if it can be consolidated into a PR. It's a simple fix with a menu option. I also see many players want this behavior. Separating it from demo compatibility/netplay/etc. is a reasonable solution to me.

As far as concerns about feature creep, in this case I see minimal risk as the code has originated and been tested elsewhere.

El-Juancho commented 2 years ago

I would love to have this fix in dsda-doom, the blockmap bug sometimes ruins some moments when im playing casually so having an option to fix this would be awesome

kraflab commented 2 years ago

I see minimal risk as the code has originated and been tested elsewhere.

The other ports don't have the same feature set as this port, so the interactions between it and any other feature are by definition untested. I'm not saying I expect there to be a bug, but doom source ports are not well compartmentalized when it comes to features, and the heavy use of global variables makes almost anything possible :^)

As far as feature creep: you are either adding features or not. If we add one small feature, then we are going to add a hundred small features over time. This is just not compatible with the concept of maintenance mode, no matter how small the feature seems. And this one is a change to a critical defining feature of doom, so I don't consider it a small feature at all.

I would love to have this fix in dsda-doom

This is unlikely. I don't consider it a problem to be fixed, and I don't want there to be such a critical difference between casual play and accurate play. There may be some legacy things like this that already existed and were inherited, but I don't plan to add more. For one thing, players and mappers testing in a casual environment may miss sections that have frequent blockmap problems :^)

It looks like it's being added to woof, so that would be a good option if it's important for you.

SirBofu commented 2 years ago

Gah, @kraflab , I wish I'd seen your comment before opening a feature request in the DSDA project. That being said,

For one thing, players and mappers testing in a casual environment may miss sections that have frequent blockmap problems :^)

This is a user issue and I think it would work itself out with experience. Mappers/testers should know not to test with this setting on unless they specifically want the player to enable it, just as they should know not to test Boom-maps exclusively in GZDoom.

That being said, this is a highly requested feature, and I am performing testing in Woof. So far, demo playback has not been an issue in Woof; I've been playing demos in parallel (unmodified DSDA-Doom in one window, modified Woof PR in another) and have seen no desyncs so far. Testing will probably need to include larger demos on larger maps, however, which is a process in and of itself.

Also, I would posit that jumping and freelook are far more disruptive changes to casual play than this would be.

MattFright commented 2 years ago

For one thing, players and mappers testing in a casual environment may miss sections that have frequent blockmap problems :^)

As a mapper and being friends with and talking with many mappers on the regular, i can confirm this isn't a thing that's ever accounted for in the first place (especially since moving a map around messes up all flat alignments in anything but UDMF), regardless of mapper experience or target audience, either. And i'd find it very hard to find any complains from players over this sort of issue whether on forums or casual discussions elsewhere, either.

And if this is a fix that can be done, i honestly would rather not have to worry about the blockmap neither as a mapper or a player :p

JadingTsunami commented 2 years ago

I've asked around a bit. Among the player base I spoke with, there is strong support for this for casual play, even if it is strictly non-demo compatible. At first I was against this without a new complevel but the combination of the arguments made in this thread, the strong player support for this, and the availability of a reasonable solution (which other ports are adopting even now) have changed my view.

If an acceptable PR is submitted, I would volunteer to provide developer support for the feature if that is the sticking point. I don't think this feature is of much interest to me personally (I would not play with it enabled), but I am willing to support it if it improves the "quality of life" for the players.

kraflab commented 2 years ago

This all comes down to whether or not this port is in maintenance mode. The discussion of how big or small this change is isn't relevant.

Coincident commented 2 years ago

I may have brought some awareness to the topic of blockmap bug; so let me add an important note:

What problem are we truly trying to fix here? The way I see it, from the point of view of the player, the problem is this:

And unfortunately, fixing the blockmap bug alone will not truly solve the problem. Random/unfair perceived misses may have other causes:

You would need to fix all 3 of these issues, and perhaps even more collision-related issues that I don't recall at the moment, to truly solve the problem from the perspective of the player. When taking into account the clutter, extra complexity, demo compatibility concerns, cross-source-port compatibility issues, player/mapper expectations, and other complex topics, I don't feel that the benefit of the fixes outweigh all these "hidden costs" that are so easy to overlook when thinking about this problem. So I totally see the value of not touching this issue, for the sake of simplicity.

I truly don't feel that for a source port like PrBoom (or any other based on PrBoom) would be the place to begin changes of this nature, given their focus on keeping retro-compatible, vanilla-faithful gameplay and demo playback. I can only recommended switching to ZDoom or one of the other source ports recommended above (that I'm admittedly less familiar with). There are some great other ports out there; and there's nothing wrong with using more than one.

SirBofu commented 2 years ago

And unfortunately, fixing the blockmap bug alone will not truly solve the problem.

Respectfully, autoaim quirks and melee weirdness are much less intrusive, and the former can already be circumvented with options that don't come into play when recording or playing demos. We are specifically talking about when shots miss an enemy you are aiming at because their center exists near the boundary of the blockmap.

The hitscan detection is a much more severe issue than either of those two other issues you're mentioning, particularly when fighting large enemies such as Spider Masterminds. And large enemies are usually the ones you don't want to miss.

We've already considered the topic of demo recording and playback over in Woof, too. It is very, very trivial to skip the extra hitscan/collision checks while playing demos and prevent desynchronization, as when the game is in a demo state, certain booleans are enabled that can easily be added into an if clause. The code that differentiates between the classic blockmap behavior and the fix is executed after the classic check already happens; it can be neatly segmented off and won't even be executed.

As for why people would want this in PrBoom+: Woof is going to support this as an option, but Woof doesn't have hardware rendering. GZDoom has its own version of RNG and introduced a number of changes that can't be disabled via menu options (Lost Souls counting toward kill counts, etc). I think the idea is that these ports already support much more wide-reaching gameplay changes in their non-strict-compatibility modes.

The beauty of open-source software, of course, is that anyone could theoretically make their own fork and add this if they want. And that's fine. But there is definitely a lot of legitimate interest in this particular option, the solution is workable and (as of current testing) doesn't impact functionality for those who don't have any desire to use it.

If this project isn't going to add any new functionality whatsoever and just focus on performance/hardening, that's the project owners' decision, of course. But this suggestion is definitely an achievable short-term goal; perhaps it can live in a branch for impact testing rather than a full-fledged fork.