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

libtrx/anims/frames: migrate TR2 frame handling to TRX #2246

Merged
merged 3 commits into from
Jan 11, 2025

Conversation

lahm86
Copy link
Collaborator

@lahm86 lahm86 commented Jan 10, 2025

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change
  • I have added a readme entry about my new feature or OG bug fix, or it is a different change

Description

This moves TR2 frame handling into TRX. We retain the original (super)packed rotations for the time being, this will be tackled on its own. Apologies that this is mostly one fairly large commit - everything was so dependent I couldn't see a way to split it up further.

There is a workaround added for objects like the prayer wheel, which do not have any animations. Because we set the object's frame base while iterating animations, the wheel remained with a null pointer. The workaround is to store the frame offset while reading the level, then later point it to the matching animation (and to clarify, the wheel points to the flare pack's animation - we will fix this in the future with injections).

I've removed frame_base from the savegame (well, we're now skipping reading and just writing a 0) in line with TR1. This is set on each frame anyway and I've confirmed by saving in TR2X while using a gun for example, then loading it in OG, plus vice-versa. I'd appreciate some other tests around saving/loading while Lara's arms are busy with guns/flares.

Once this lands, we can migrate TR2's BOUNDS16 to match TR1, which means we can streamline things like M_ParseFrame.

@lahm86 lahm86 added Internal The invisible stuff TR2 labels Jan 10, 2025
@lahm86 lahm86 self-assigned this Jan 10, 2025
@lahm86 lahm86 requested review from a team as code owners January 10, 2025 14:50
@lahm86 lahm86 requested review from rr-, walkawayy and aredfan and removed request for a team January 10, 2025 14:50
Copy link

github-actions bot commented Jan 10, 2025

src/libtrx/game/level/common.c Show resolved Hide resolved
src/tr2/game/level.c Show resolved Hide resolved
@lahm86 lahm86 requested a review from rr- January 10, 2025 15:25
GameBuf_Alloc(sizeof(int16_t) * (frame_size - 9), GBUF_ANIM_FRAMES);
memcpy(frame->mesh_rots, data_ptr, sizeof(int16_t) * (frame_size - 9));
data_ptr += MAX(0, frame_size - (data_ptr - frame_start));
#else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if test in the code below in the else still needed? Because the if above already checks for TR_VERSION > 1

    #if TR_VERSION == 1
    mesh_count = *data_ptr++;
    #endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be tidying up this function after this, it's a bit untidy atm until we get TR2 rotations expanded and BOUNDS_16 working the same as TR1. The #ifs will become neater then. I could remove now but would re-introduce it more or less immediately anyway.

@aredfan
Copy link
Collaborator

aredfan commented Jan 10, 2025

I've tested with swapping saves between TR2X and OG, all LGTM.

I also found a small issue in the PR build that isn't present in latest dev snapshot, and similar to #1906. It looks like the mercenary's shot is hitting Lara in a different spot which causes the blood splatter to shift to another part of the screen.

PR:
1 1 PR

Dev snapshot:
1 2 Dev

PR:
2 1 PR

Dev snapshot:
2 2 Dev

@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 10, 2025

Thank @aredfan. I think this is a nasty bug, and I believe OG might actually be incorrect. Tagging @rr, @walkawayy for thoughts on this.

I've done some logging on the dev snapshot and this PR, and you're exactly right, the blood is spawning in a different place. Everything else matches up.

PR
Goon pos: 40523 1427 51005
Lara pos: 40209 1349 48485
Goon gun spark pos: 40342 919 49703
Lara blood pos: 40245 802 48588

Dev
Goon pos: 40523 1427 51005
Lara pos: 40209 1349 48485
Goon gun spark pos: 40342 919 49703
Lara blood pos: 40303 736 48392

The issue I believe is in Spawn_GunHit. On logging (in both builds), the joint passed to Collide_GetJointAbsPosition is 17 (so that's the result of Random_GetControl() * 25 / 0x7FFF). Now this is wrong because Lara only has 15 joints 😨 So that function is going through all of Lara's joints and then blindly moving on twice more to whatever is next in memory in terms of bones and frames. Because we have changed to read exactly the amount of frames needed, we're now getting something different. TR2 frames are padded, but because we are now reading from frame offset per anim, and reading only what we need, the result of blindly reading too far in things like Collide_GetJointAbsPosition is that we spill into another frameset rather than first hitting padding and then potentially another frameset.

I don't think we can match OG here if we are going to have structured frames in all honesty. Once we get to expanding frames, the padding is even more meaningless because we don't know what to assign it as.

@walkawayy
Copy link
Collaborator

Hm so this has the potential to affect where blood spawns in normal gameplay as well, not just demos? It does seem like a bug that Random_GetControl() * 25 / 0x7FFF can produce a joint number bigger than Lara's total. Can we change this formula to make sure it's between 0-15 or whatever it needs to be? The point of the function seems to be that it randomly spawns blood on one of Lara's joints, so it doesn't seem like an issue to me to correct the function to pick a proper joint number. Maybe rr will have more thoughts.

Btw it also looks like it could affect TR1 since it uses the same formula:

g_LaraItem, &pos, (Random_GetControl() * 25) / 0x7FFF);

@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 10, 2025

Yeah, it's a TR1 issue too. But because frames aren't padded there we're still getting the same results as before, but still, we are reading into data we shouldn't be.
I've tried clamping the value to 15 and when Lara's standing still, you end up with seemingly "stickier" blood splats, as in they are precisely at whichever joint they spawn at. It's better with the following, we can likely play around with the values if we decide to fix this.

diff --git a/src/tr2/game/collide.c b/src/tr2/game/collide.c
index 5c76d797..0eb61e83 100644
--- a/src/tr2/game/collide.c
+++ b/src/tr2/game/collide.c
@@ -579,7 +579,9 @@ void Collide_GetJointAbsPosition(
     Matrix_RotYXZsuperpack(&mesh_rots, 0);
 
     const int16_t *extra_rotation = item->data;
-    for (int32_t i = 0; i < joint; i++) {
+    int32_t abs_joint = joint;
+    CLAMP(abs_joint, 0, object->mesh_count);
+    for (int32_t i = 0; i < abs_joint; i++) {
         const ANIM_BONE *const bone = Object_GetBone(object, i);
         if (bone->matrix_pop) {
             Matrix_Pop();
diff --git a/src/tr2/game/spawn.c b/src/tr2/game/spawn.c
index ba64ac86..1521a59a 100644
--- a/src/tr2/game/spawn.c
+++ b/src/tr2/game/spawn.c
@@ -143,9 +143,13 @@ int16_t Spawn_GunHit(
     const int32_t x, const int32_t y, const int32_t z, const int16_t speed,
     const int16_t y_rot, const int16_t room_num)
 {
-    XYZ_32 vec = {};
+    XYZ_32 vec = {
+        .x = 50 * (Random_GetDraw() - 0x4000) / 0x4000,
+        .y = 50 * (Random_GetDraw() - 0x4000) / 0x4000,
+        .z = 50 * (Random_GetDraw() - 0x4000) / 0x4000,
+    };
     Collide_GetJointAbsPosition(
-        g_LaraItem, &vec, Random_GetControl() * 25 / 0x7FFF);
+        g_LaraItem, &vec, Random_GetControl() * LM_NUMBER_OF / 0x7FFF);
     Spawn_Blood(
         vec.x, vec.y, vec.z, g_LaraItem->speed, g_LaraItem->rot.y,
         g_LaraItem->room_num);

@rr-
Copy link
Collaborator

rr- commented Jan 10, 2025

Don't the additional Random() calls cause the OG demos to desync?

@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 10, 2025

It would change visuals only as it's Random_GetDraw() rather than control, I think? We could add a check to only do it in normal gameplay. But in any case, the blood splats will differ from OG anyway if we clamp to 15. Just something to consider because the bug aredfan found before at this spot led to discovering the early FD triggering issue, so it's definitely a benefit comparing demos across builds. If we proceed, we'd need to mark a new baseline build for comparing demos in the future.

@rr-
Copy link
Collaborator

rr- commented Jan 11, 2025

It would change visuals only as it's Random_GetDraw() rather than control, I think? We could add a check to only do it in normal gameplay. But in any case, the blood splats will differ from OG anyway if we clamp to 15. Just something to consider because the bug aredfan found before at this spot led to discovering the early FD triggering issue, so it's definitely a benefit comparing demos across builds. If we proceed, we'd need to mark a new baseline build for comparing demos in the future.

I can't think of any other way out of this. Let's proceed, as long as the rest of the demos appear normal.

@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 11, 2025

Will PR separately and rebase this one later.

The frame_base is not required to be saved or loaded for Lara's arms,
because it is set on each frame anyway. Removes a dependency on
g_AnimFrames, similar to TR1.
This migrates animation frame storage to TRX and changes the way frames
are interpreted. All previous casting to ANIM_FRAME is now handled
normally throughout the TR2 codebase. Rotations remain unparsed, this
will be tackled separately.
This removes the separate checks for handling interpolation and frame
size on animations.
@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 11, 2025

Rebased, @aredfan, if you fancy doing a check. Develop build below on left, PR on right.

image

Copy link
Collaborator

@aredfan aredfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased, @aredfan, if you fancy doing a check. Develop build below on left, PR on right.

Np and happy to check, I can confirm the position Lara gets shot matches your screenshots. I understand the demo will be slightly different visually and that's perfectly fine.

LGTM, and thank you again. 👍

@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 11, 2025

Rebased, @aredfan, if you fancy doing a check. Develop build below on left, PR on right.

Np and happy to check, I can confirm the position Lara gets shot matches your screenshots. I understand the demo will be slightly different visually and that's perfectly fine.

LGTM, and thank you again. 👍

Thanks. This was a great find, good to get it fixed 😌

@lahm86 lahm86 merged commit b7eaf73 into LostArtefacts:develop Jan 11, 2025
7 checks passed
@lahm86 lahm86 deleted the tr2-frames branch January 11, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal The invisible stuff TR2
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants