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: expand TR1 frame rotations #2244

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

lahm86
Copy link
Collaborator

@lahm86 lahm86 commented Jan 9, 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 parses TR1 frame mesh rotations fully once on level load, which means we eliminate the need to pass packed values to matrix.c and have it extract the individual values on every call.

I'm not sure if I like the name Matrix_RotYXZ_16. I can see potential though to change Matrix_RotYXZ's signature in a follow-up task to accept an XYZ_16 instead of the individual values.

The extraction macros are a little more complex ready for TR2, as it won't always have two int16_ts so we can't cast to int32_t and use the previous approach. Similarly, the M_ParseMeshRotation function returns the number of values read because of what's to come with TR2.

This also removes nmeshes from ANIM_FRAME - now taken from the related object.

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

github-actions bot commented Jan 9, 2025

@@ -5,11 +5,17 @@
#include "game/objects/common.h"
#include "log.h"

#define EXTRACT_ROT_X(r) ((r & 0x3FF0) << 2)
#define EXTRACT_ROT_Y(r1, r2) ((((r1 & 0xF) << 6) | ((r2 & 0xFC00) >> 10)) << 6)
#define EXTRACT_ROT_Z(r) ((r & 0x3FF) << 6)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's forgo these macros and put the code directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kinda nice to have the nice defined names though. Because that code is ugly and dense to read. 😆

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've create a M_ExtractRotation function. For TR2 rots with two values, we will still need to utilise this but its words are reversed so a function will avoid repetition when the time comes.

ASSERT_FAIL();
return 0;
#endif
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function could take a double pointer removing the need for a return value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -20,7 +20,7 @@ void Matrix_RotX(int16_t rx);
void Matrix_RotY(int16_t ry);
void Matrix_RotZ(int16_t rz);
void Matrix_RotYXZ(int16_t ry, int16_t rx, int16_t rz);
void Matrix_RotYXZpack(int32_t rots);
void Matrix_RotYXZ_16(const XYZ_16 *rotation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just call it Rot or RotXYZ as the order doesn't matter anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also ok with XYZ16, as underscores are used in a different way in our function names, and this nane would clearly distinguish this function's signature from, say, TranslateRel which take individual components still.

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've gone with Matrix_RotXYZ16, agreed it keeps it clear.

@aredfan
Copy link
Collaborator

aredfan commented Jan 10, 2025

  • The game crashes when trying to load Lara's Home or Tomb of Qualopec.

This uses the object's mesh count for reading the number of rotations
per frame, although for TR1 we retain using what's defined in the
frame.
This parses TR1 frame mesh rotations fully once on level load, which
means we eliminate the need to pass packed values to matrix.c and have
it extract the values on every call.
@lahm86
Copy link
Collaborator Author

lahm86 commented Jan 10, 2025

@aredfan Thanks, fixed

@lahm86 lahm86 requested a review from rr- January 10, 2025 09:42
Copy link
Collaborator

@rr- rr- left a comment

Choose a reason for hiding this comment

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

LGTM. Kickass changes. Thank you.

@lahm86 lahm86 merged commit fffe6fd into LostArtefacts:develop Jan 10, 2025
7 checks passed
@lahm86 lahm86 deleted the tr1-mesh-rots branch January 10, 2025 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal The invisible stuff TR1
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants