Ifdef several collision changes from vanilla + improved debug free move #794
Conversation
… ADD_CEILING_MARGINS subdefines
… into develop/2.2.0-ifdef-collision-improvements
Arceveti
added
enhancement
| @@ -49,7 +50,11 @@ f32 find_ceil(f32 posX, f32 posY, f32 posZ, struct Surface **pceil); | |||
|
|
|||
| // Finds the ceiling from a vec3f and a minimum height (with 3 unit vertical buffer). | |||
| ALWAYS_INLINE f32 find_mario_ceil(Vec3f pos, f32 height, struct Surface **ceil) { | |||
| return find_ceil(pos[0], MAX(height, pos[1]) + 3.0f, pos[2], ceil); | |||
| #ifdef FIX_EXPOSED_CEILINGS | |||
There was a problem hiding this comment.
this is an ugly way to do this, id rather you just have height = MAX(height, pos[1]) + 3.0f; in an ifdef
There was a problem hiding this comment.
Done, but I moved the + 3.0f outside the ifdef since vanilla also has the offset.
| @@ -1135,7 +1137,7 @@ void mode_8_directions_camera(struct Camera *c) { | |||
| } | |||
| #ifdef PARALLEL_LAKITU_CAM | |||
| // extra functionality | |||
| else if (gPlayer1Controller->buttonPressed & U_JPAD) { | |||
| else if ((gPlayer1Controller->buttonPressed & U_JPAD) && (gMarioState->action != ACT_DEBUG_FREE_MOVE)) { | |||
There was a problem hiding this comment.
we should not really have camera-specific behavior for debug free move, just wasting code and perf considering that no final release will have it
There was a problem hiding this comment.
I disagree since the camera interference is extremely annoying in practice, however idk if this is the best way to fix it.
There was a problem hiding this comment.
what interference? this code is only when dpad up is being pressed. just dont press dpad up? its so pointless
There was a problem hiding this comment.
D-Pad Up is used to move up in debug free mode (and enter the mode in the first place), that's pretty damn important.
There was a problem hiding this comment.
This check should also be applied to D_JPAD actually
| @@ -6,6 +6,7 @@ | |||
| #include "surface_collision.h" | |||
| #include "types.h" | |||
|
|
|||
|
|
|||
include/config/config_movement.h
Outdated
| * Also properly handles simultaneous collisions with multiple walls (eg. concave wall corners or narrow tunnels). | ||
| */ | ||
| #define ROUNDED_WALL_CORNERS | ||
| /** |
include/config/config_movement.h
Outdated
|
|
||
| /** | ||
| * Improves the handling of convex wall corners by rounding wall collision at triangle edges to close the seams. | ||
| * Also properly handles simultaneous collisions with multiple walls (eg. concave wall corners or narrow tunnels). | ||
| */ | ||
| #define ROUNDED_WALL_CORNERS | ||
| /** | ||
| * Fixes an issue where entering an area above a ceiling without an intermediate floor would count as hitting a ceiling. | ||
| * NOTE: This may allow Mario to clip through the wall on the deck of the the rocking JRB ship. | ||
| */ | ||
| #define FIX_EXPOSED_CEILINGS |
There was a problem hiding this comment.
Wouldn't it make more sense to throw these into config_collision.h? Argument could be made for ceiling bonks too but that one's debatable.
| @@ -257,6 +257,7 @@ enum SurfaceFlags { | |||
| SURFACE_FLAGS_NONE = (0 << 0), // 0x0000 | |||
| SURFACE_FLAG_DYNAMIC = (1 << 0), // 0x0001 | |||
| SURFACE_FLAG_NO_CAM_COLLISION = (1 << 1), // 0x0002 | |||
| SURFACE_FLAG_X_PROJECTION = (1 << 3), // 0x0008 | |||
There was a problem hiding this comment.
That's how it was in vanilla, this just restores it. Should it be changed to (1 << 2)?
There was a problem hiding this comment.
Yeah it just makes more sense
| /************************************************** | ||
| * WALLS * | ||
| **************************************************/ | ||
|
|
||
| static s32 check_wall_vw(f32 d00, f32 d01, f32 d11, f32 d20, f32 d21, f32 invDenom) { | ||
| #ifdef ROUNDED_WALL_CORNERS | ||
| static s32 check_wall_triangle_vw(f32 d00, f32 d01, f32 d11, f32 d20, f32 d21, f32 invDenom) { |
There was a problem hiding this comment.
This code is old so won't enforce the change, but these variable names in general are not great.
src/engine/surface_collision.c
Outdated
| if (ceil->flags & SURFACE_FLAG_NO_CAM_COLLISION) continue; | ||
| } else { | ||
| // If we are not checking for the camera, ignore camera only ceilings. | ||
| if (ceil->type == SURFACE_CAMERA_BOUNDARY) continue; | ||
| } | ||
|
|
||
| // Check that the point is within the triangle bounds | ||
| if (!check_within_ceil_triangle_bounds(x, z, surf, 1.5f)) continue; | ||
| // Exclude all ceilings below the check height. | ||
| if (y > ceil->upperY) continue; | ||
|
|
||
| // Find the height of the ceil at the given location | ||
| height = get_surface_height_at_location(x, z, surf); | ||
|
|
||
| // Exclude ceilings above the previous lowest ceiling | ||
| if (height > *pheight) continue; | ||
| // Check that the point is within the triangle bounds. | ||
| if (!check_within_ceil_triangle_bounds(x, z, ceil)) continue; |
There was a problem hiding this comment.
Nit: Inlining continue/break/return is fine if it's part of a consistent and repetitive code block, but otherwise it's terrible coding practice. Anything that significantly impacts processing in that way needs to be more explicit (i.e. have its own line, ideally with a comment if it's not braindead obvious).
Calling this one a nitpick since it's mostly not new code, but I still do not like it in general.
There was a problem hiding this comment.
I changed most of these to be on their own lines (except for some of the repetitive ones like the vanilla wall collisions or the check floor/ceil triangle bounds functions).
src/engine/surface_collision.c
Outdated
| // Exclude ceiling heights that are: | ||
| if ( | ||
| (ceilHeight < y) || // Lower than the check height. | ||
| (ceilHeight >= lowestCeilHeight) // Higher than the previous lowest ceiling. | ||
| ) continue; |
There was a problem hiding this comment.
Except not for this one, this just looks bad lol
There was a problem hiding this comment.
I'm not sure if you mean the if statement or the continue here, but I put the continue on its own line here too.
There was a problem hiding this comment.
Specifically the continue; the if statement itself doesn't bother me
| @@ -394,10 +436,11 @@ f32 find_ceil(f32 posX, f32 posY, f32 posZ, struct Surface **pceil) { | |||
| } | |||
|
|
|||
| // To prevent accidentally leaving the floor tangible, stop checking for it. | |||
| gCollisionFlags &= ~(COLLISION_FLAG_RETURN_FIRST | COLLISION_FLAG_EXCLUDE_DYNAMIC | COLLISION_FLAG_INCLUDE_INTANGIBLE); | |||
| gCollisionFlags &= ~(COLLISION_FLAG_EXCLUDE_DYNAMIC | COLLISION_FLAG_INCLUDE_INTANGIBLE); | |||
There was a problem hiding this comment.
Curious, have these been profiled? Seems like there's more than I realized.
| if (checkCollisions) { | ||
| if ((ceilHeight - floorHeight) < 160.0f) { | ||
| return FALSE; | ||
| } | ||
| if ((floor != NULL) && (pos[1] < floorHeight)) { | ||
| pos[1] = floorHeight; | ||
| } | ||
| if (ceil != NULL && pos[1] + 160.0f > ceilHeight) { | ||
| if ((ceil != NULL) && ((pos[1] + 160.0f) > ceilHeight) && (pos[1] < ceilHeight)) { | ||
| pos[1] = ceilHeight - 160.0f; | ||
| } | ||
| vec3f_copy(m->pos, pos); | ||
| } |
There was a problem hiding this comment.
Nit: I preferred the general look of the old way better, generally not a fan of holding the entire function captive to an early return in the event somebody wanted to add logic onto this for example.
src/game/object_list_processor.h
Outdated
| COLLISION_FLAG_CAMERA = (1 << 1), | ||
| COLLISION_FLAG_INCLUDE_INTANGIBLE = (1 << 2), | ||
| COLLISION_FLAG_EXCLUDE_DYNAMIC = (1 << 3), |
There was a problem hiding this comment.
Any reason these skip (1 << 0)?
There was a problem hiding this comment.
Not sure, either a typo or there used to be a flag there that was removed. I moved exclude dynamic to be (1 << 0).
gheskett
added
the
high priority
Makes several collision changes from vanilla no longer enforced.
Added defines:
DISABLE_CEILING_BONKSROUNDED_WALL_CORNERSFIX_EXPOSED_CEILINGSAlso:
add_ceil_margin. It's a leftover from the original patch and was supposed to be a bandaid fix for exposed ceilings by making them slightly smaller, but due to a typo it had no effect in HackerSM64.COLLISION_FLAG_RETURN_FIRST. This was supposed to allow calling find_floor/find_ceil with/without sorting, but there aren't any use cases that are worth keeping an additional check in those functions.