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

Reworked Z movement in Tick() to be more reliable #2303

Closed
wants to merge 6 commits into from

Conversation

Boondorl
Copy link
Contributor

GZDoom's Z movement for the longest time has been fairly lackluster, but there are common edge cases that show just how egregiously behind it truly is. Case-in-point, projectiles are missing almost every single one of their checks, including SpecialMissileHit and any ghost properties. In the example below, I've given the ZombieMan +GHOST and the rocket, a now-bouncing projectile, +THRUGHOST. The radius of the ZombieMan was increased to highlight just how noticeable this case is in practice. If you have a BounceCount set, this cases them to blow up almost instantly:

Broken.Showcase.mp4

This has now been fixed by using CheckPosition directly. The reason collisions would not register on the Z axis before was due to the stuck prevention logic taking priority. A parameter has been added to ignore this since, while doing Z checks, we obviously don't care about this as it's mainly meant for XY movement. This causes it to correctly register collisions. On top of this, the functionality from XY movement on certain collisions related to bouncing and reflecting was copied in, giving parity in features on all 3 axes. Fixed projectile behavior:

Fixed.Showcase.mp4

The result of this is that TestMobjZ is being deprecated. This function is clearly not being maintained as is and, as of now, is functionally worse than CheckPosition in every way. Rather than maintain two copies of what is essentially the exact same code, I've instead opted to simply throw it away in favor of the new parameter. This will make maintaining this part of the engine significantly easier going into the future.

As a final bonus, GroundMobj has been added as a pointer since the flag by itself is borderline useless to modders. This gets updated to whatever Actor the faller landed on top of. This has significantly more applications for gameplay than having to check bOnMobj and then running a TestMobjZ again to grab it.

Added checkUnblock parameter to CheckPosition to allow disabling of stuck prevention when doing Z checks. Added GroundMobj pointer to give direct access to what's being stood on top of. Added missing projectile and bouncer checks after doing a Z movement. Deprecated TestMobjZ since CheckPosition is now superior.
@Boondorl Boondorl marked this pull request as draft December 17, 2023 08:16
@coelckers
Copy link
Member

As this is an extremely invasive change you have to do some extensive tests and prove that this doesn't break things.
I know that z movement is not in the best state but this code is 20 years old and who knows how many maps depend on some of these quirks.

@madame-rachelle
Copy link
Collaborator

In that case, I think it's better to just throw it into a compatibility option and be done with it. The change may be invasive but I think it's necessary, and depending on the old movement code is something that I expect will be very uncommon.

I've seen Boondorl post video tests with the old movement code, particularly on 3D Floor slopes, and I can definitely tell you that this change is desperately needed.

@coelckers
Copy link
Member

We still have to be extremely careful with P_CheckPosition because this function can trigger actions outside the calling actor, so if it is supposed to stand in for P_TestMobjZ it needs to be aware of this case not triggering any such action.

Added P_TestMobjCollision as a replacement for P_TestMobjZ. Shifted over remaining P_TestMobjZ calls to the replacement function. Fixed a longstanding bug where Actors bumping their head into another Actor would consider them on the ground for that tic. If trying to step up an Actor, the collision check is no longer ran twice.
@Boondorl
Copy link
Contributor Author

Boondorl commented Dec 17, 2023

I'm drafting this for now because I'm aware this is a large change to how the Z physics work and expect it to need extensive testing. One issue is that TestMobjZ is so underwhelming it's difficult to tell what was and wasn't intentional. It also leaves a gaping discrepancy between how the Z and XY axes function. The XY axes cannot check something without triggering all side effects. This is a case of CheckPosition being poorly implemented (or rather, poorly named), but the proper solution here is to fix this within CheckPosition itself, not split it off into another function that inevitably doesn't get maintained

Something worth noting is that this doesn't actually fix the problem with projectiles and bouncing on 3D floors. That's a different fix. The amount of things TestMobjZ misses though is astounding. Compare it to the checking PIT_CheckThing does and it's a night and day difference. Some of the behaviors built into PIT_CheckThing were even ripped out and put in the Z movement check making the whole thing feel redundant.

Over the weeks I'll be combing through CheckPosition to see if anything needs to be modified. It's likely there are actually a ton of bugs related to the old behavior this used. For instance, TestMobjLocation won't detect basically any Actors since the test position is the same as the caller's current position and the stuck prevention mechanism kicks in. Is this intentional? Who knows, but it certainly doesn't seem like it, especially since the comment above it mentions anything blocking it.

Fixed an error where the height check when stepping on an Actor would look at its Z position and not the Actor's Z at their top. Blasting will now apply when hitting the top/bottom of an Actor. Disabled pushing when doing a Z movement.
Things must now be within step height to be considered on top of.
Now recalculates the mobj since P_CheckPosition and P_TestMobjZ register valid hits differently.
@Boondorl
Copy link
Contributor Author

Gonna close this for now as the scope has changed after investigating and I'd like to break it down into smaller PRs over a longer time span

@Boondorl Boondorl closed this Apr 26, 2024
@MajorCooke
Copy link
Contributor

I look forward to it personally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants