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.

Sign up for GitHub

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

Create matrix.h and matrix_stack.h #1975

Closed
wants to merge 8 commits into from
Closed

Create matrix.h and matrix_stack.h #1975

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3c28341
create matrix.h
Dragorn421 Jun 25, 2024
c0c879b
split into matrix.h + matrix_stack.h
Dragorn421 Jun 25, 2024
d1e3b4b
Merge branch 'main' into h_matrix
Dragorn421 Jun 25, 2024
c34bc60
fix include guard...
Dragorn421 Jun 25, 2024
d35d40d
fix retail bss
Dragorn421 Jun 25, 2024
8f40621
sys_matrix.h, skin_matrx.h, and bss
Dragorn421 Jun 26, 2024
131e4f1
Merge branch 'main' into h_matrix
Dragorn421 Jun 26, 2024
3301332
revert makefile -Werror=implicit-function-declaration
Dragorn421 Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ INC := -Iinclude -Iinclude/libc -Isrc -I$(BUILD_DIR) -I. -I$(EXTRACTED_DIR)

# Check code syntax with host compiler
CHECK_WARNINGS := -Wall -Wextra -Wno-format-security -Wno-unknown-pragmas -Wno-unused-parameter -Wno-unused-variable -Wno-missing-braces
CHECK_WARNINGS += -Werror=implicit-function-declaration

# The `cpp` command behaves differently on macOS (it behaves as if
# `-traditional-cpp` was passed) so we use `gcc -E` instead.
Expand Down
55 changes: 2 additions & 53 deletions include/functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,21 +1072,7 @@ void Skin_Init(PlayState* play, Skin* skin, SkeletonHeader* skeletonHeader, Anim
void Skin_Free(PlayState* play, Skin* skin);
s32 Skin_ApplyAnimTransformations(Skin* skin, MtxF* limbMatrices, Actor* actor, s32 setTranslation);

void SkinMatrix_Vec3fMtxFMultXYZW(MtxF* mf, Vec3f* src, Vec3f* xyzDest, f32* wDest);
void SkinMatrix_Vec3fMtxFMultXYZ(MtxF* mf, Vec3f* src, Vec3f* dest);
void SkinMatrix_MtxFMtxFMult(MtxF* mfA, MtxF* mfB, MtxF* dest);
void SkinMatrix_GetClear(MtxF** mfp);
void SkinMatrix_MtxFCopy(MtxF* src, MtxF* dest);
s32 SkinMatrix_Invert(MtxF* src, MtxF* dest);
void SkinMatrix_SetScale(MtxF* mf, f32 x, f32 y, f32 z);
void SkinMatrix_SetRotateZYX(MtxF* mf, s16 x, s16 y, s16 z);
void SkinMatrix_SetTranslate(MtxF* mf, f32 x, f32 y, f32 z);
void SkinMatrix_SetTranslateRotateYXZScale(MtxF* dest, f32 scaleX, f32 scaleY, f32 scaleZ, s16 rotX, s16 rotY, s16 rotZ,
f32 translateX, f32 translateY, f32 translateZ);
void SkinMatrix_SetTranslateRotateZYX(MtxF* dest, s16 rotX, s16 rotY, s16 rotZ, f32 translateX, f32 translateY,
f32 translateZ);
Mtx* SkinMatrix_MtxFToNewMtx(GraphicsContext* gfxCtx, MtxF* src);
void SkinMatrix_SetRotateAxis(MtxF* mf, s16 angle, f32 axisX, f32 axisY, f32 axisZ);

void Sram_InitNewSave(void);
void Sram_InitDebugSave(void);
void Sram_OpenSave(SramContext* sramCtx);
Expand Down Expand Up @@ -1264,42 +1250,7 @@ void* SysCfb_GetFbEnd(void);

void Math3D_DrawSphere(PlayState* play, Sphere16* sph);
void Math3D_DrawCylinder(PlayState* play, Cylinder16* cyl);
void Matrix_Init(GameState* gameState);
void Matrix_Push(void);
void Matrix_Pop(void);
void Matrix_Get(MtxF* dest);
void Matrix_Put(MtxF* src);
void Matrix_Mult(MtxF* mf, u8 mode);
void Matrix_Translate(f32 x, f32 y, f32 z, u8 mode);
void Matrix_Scale(f32 x, f32 y, f32 z, u8 mode);
void Matrix_RotateX(f32 x, u8 mode);
void Matrix_RotateY(f32 y, u8 mode);
void Matrix_RotateZ(f32 z, u8 mode);
void Matrix_RotateZYX(s16 x, s16 y, s16 z, u8 mode);
void Matrix_TranslateRotateZYX(Vec3f* translation, Vec3s* rotation);
void Matrix_SetTranslateRotateYXZ(f32 translateX, f32 translateY, f32 translateZ, Vec3s* rot);
Mtx* Matrix_MtxFToMtx(MtxF* src, Mtx* dest);
#if OOT_DEBUG
Mtx* Matrix_ToMtx(Mtx* dest, const char* file, int line);
Mtx* Matrix_NewMtx(GraphicsContext* gfxCtx, const char* file, int line);
#else
Mtx* Matrix_ToMtx(Mtx* dest);
Mtx* Matrix_NewMtx(GraphicsContext* gfxCtx);
#endif
void Matrix_MultVec3f(Vec3f* src, Vec3f* dest);
void Matrix_MtxFCopy(MtxF* dest, MtxF* src);
void Matrix_MtxToMtxF(Mtx* src, MtxF* dest);
void Matrix_MultVec3fExt(Vec3f* src, Vec3f* dest, MtxF* mf);
void Matrix_Transpose(MtxF* mf);
void Matrix_ReplaceRotation(MtxF* mf);
void Matrix_MtxFToYXZRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
void Matrix_MtxFToZYXRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
void Matrix_RotateAxis(f32 angle, Vec3f* axis, u8 mode);
#if OOT_DEBUG
MtxF* Matrix_CheckFloats(MtxF* mf, const char* file, int line);
#endif
void Matrix_SetTranslateScaleMtx2(Mtx* mtx, f32 scaleX, f32 scaleY, f32 scaleZ, f32 translateX, f32 translateY,
f32 translateZ);

u64* SysUcode_GetUCodeBoot(void);
size_t SysUcode_GetUCodeBootSize(void);
u64* SysUcode_GetUCode(void);
Expand Down Expand Up @@ -1549,8 +1500,6 @@ s32 GfxPrint_Printf(GfxPrint* this, const char* fmt, ...);
void RcpUtils_PrintRegisterStatus(void);
void RcpUtils_Reset(void);
void* Overlay_AllocateAndLoad(uintptr_t vromStart, uintptr_t vromEnd, void* vramStart, void* vramEnd);
void MtxConv_F2L(Mtx* m1, MtxF* m2);
void MtxConv_L2F(MtxF* m1, Mtx* m2);
void Overlay_Relocate(void* allocatedRamAddr, OverlayRelocationSection* ovlRelocs, void* vramStart);
size_t Overlay_Load(uintptr_t vromStart, uintptr_t vromEnd, void* vramStart, void* vramEnd, void* allocatedRamAddr);
// ? func_800FC800(?);
Expand Down
6 changes: 0 additions & 6 deletions include/macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,6 @@ extern struct GraphicsContext* __gfxCtx;
(void)0

#define GRAPH_ALLOC(gfxCtx, size) Graph_Alloc(gfxCtx, size)
#define MATRIX_TO_MTX(gfxCtx, file, line) Matrix_ToMtx(gfxCtx, file, line)
#define MATRIX_NEW(gfxCtx, file, line) Matrix_NewMtx(gfxCtx, file, line)
#define MATRIX_CHECK_FLOATS(mtx, file, line) Matrix_CheckFloats(mtx, file, line)
#define DMA_REQUEST_SYNC(ram, vrom, size, file, line) DmaMgr_RequestSyncDebug(ram, vrom, size, file, line)
#define DMA_REQUEST_ASYNC(req, ram, vrom, size, unk5, queue, msg, file, line) DmaMgr_RequestAsyncDebug(req, ram, vrom, size, unk5, queue, msg, file, line)
#define GAME_STATE_ALLOC(gameState, size, file, line) GameState_Alloc(gameState, size, file, line)
Expand Down Expand Up @@ -216,9 +213,6 @@ extern struct GraphicsContext* __gfxCtx;
(void)0

#define GRAPH_ALLOC(gfxCtx, size) ((void*)((gfxCtx)->polyOpa.d = (Gfx*)((u8*)(gfxCtx)->polyOpa.d - ALIGN16(size))))
#define MATRIX_TO_MTX(gfxCtx, file, line) Matrix_ToMtx(gfxCtx)
#define MATRIX_NEW(gfxCtx, file, line) Matrix_NewMtx(gfxCtx)
#define MATRIX_CHECK_FLOATS(mtx, file, line) (mtx)
#define DMA_REQUEST_SYNC(ram, vrom, size, file, line) DmaMgr_RequestSync(ram, vrom, size)
#define DMA_REQUEST_ASYNC(req, ram, vrom, size, unk5, queue, msg, file, line) DmaMgr_RequestAsync(req, ram, vrom, size, unk5, queue, msg)
#define GAME_STATE_ALLOC(gameState, size, file, line) THA_AllocTailAlign16(&(gameState)->tha, size)
Expand Down
52 changes: 52 additions & 0 deletions include/matrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#ifndef MATRIX_H
#define MATRIX_H

#include "ultra64.h"
#include "z64math.h"

struct GraphicsContext;

extern Mtx gMtxClear;
extern MtxF gMtxFClear;

Mtx* Matrix_MtxFToMtx(MtxF* src, Mtx* dest);
void Matrix_MtxFCopy(MtxF* dest, MtxF* src);
void Matrix_MtxToMtxF(Mtx* src, MtxF* dest);
void Matrix_MultVec3fExt(Vec3f* src, Vec3f* dest, MtxF* mf);
void Matrix_Transpose(MtxF* mf);
void Matrix_MtxFToYXZRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
void Matrix_MtxFToZYXRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
void Matrix_SetTranslateScaleMtx2(Mtx* mtx, f32 scaleX, f32 scaleY, f32 scaleZ, f32 translateX, f32 translateY,
f32 translateZ);

#if OOT_DEBUG

MtxF* Matrix_CheckFloats(MtxF* mf, const char* file, int line);
#define MATRIX_CHECK_FLOATS(mtx, file, line) Matrix_CheckFloats(mtx, file, line)

#else

#define MATRIX_CHECK_FLOATS(mtx, file, line) (mtx)

#endif

void MtxConv_F2L(Mtx* m1, MtxF* m2);
void MtxConv_L2F(MtxF* m1, Mtx* m2);

void SkinMatrix_Vec3fMtxFMultXYZW(MtxF* mf, Vec3f* src, Vec3f* xyzDest, f32* wDest);
void SkinMatrix_Vec3fMtxFMultXYZ(MtxF* mf, Vec3f* src, Vec3f* dest);
void SkinMatrix_MtxFMtxFMult(MtxF* mfA, MtxF* mfB, MtxF* dest);
void SkinMatrix_GetClear(MtxF** mfp);
void SkinMatrix_MtxFCopy(MtxF* src, MtxF* dest);
s32 SkinMatrix_Invert(MtxF* src, MtxF* dest);
void SkinMatrix_SetScale(MtxF* mf, f32 x, f32 y, f32 z);
void SkinMatrix_SetRotateZYX(MtxF* mf, s16 x, s16 y, s16 z);
void SkinMatrix_SetTranslate(MtxF* mf, f32 x, f32 y, f32 z);
void SkinMatrix_SetTranslateRotateYXZScale(MtxF* dest, f32 scaleX, f32 scaleY, f32 scaleZ, s16 rotX, s16 rotY, s16 rotZ,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd think it makes more sense for sys_matrix and skin_matrix to be separate headers. They are different systems that aren't really compatible with each other, being that one manipulates the matrix stack and the other does not.

Take for example eff_ss_kirakira, which opts to use solely skinmatrix operations instead of the matrix stack. It should probably just be able to include the specific matrix system it wants to use on its own

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do we gain from two separate headers?

I think we should split headers by feature rather than make basically one .h per .c, otherwise we'd end up with separate headers for the same thing (like matrix operations here) or a single header with way too much going on (z_actor.c)

Do note that sys_matrix.c is not only operations on the matrix stack, it also has operations that don't involve the stack:

Mtx* Matrix_MtxFToMtx(MtxF* src, Mtx* dest);
void Matrix_MtxFCopy(MtxF* dest, MtxF* src);
void Matrix_MtxToMtxF(Mtx* src, MtxF* dest);
void Matrix_MultVec3fExt(Vec3f* src, Vec3f* dest, MtxF* mf);
void Matrix_Transpose(MtxF* mf);
void Matrix_MtxFToYXZRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
void Matrix_MtxFToZYXRotS(MtxF* mf, Vec3s* rotDest, s32 flag);
MtxF* Matrix_CheckFloats(MtxF* mf, const char* file, int line);
void Matrix_SetTranslateScaleMtx2(Mtx* mtx, f32 scaleX, f32 scaleY, f32 scaleZ, f32 translateX, f32 translateY,
                                  f32 translateZ);

I would be ok splitting the header between "matrix_stack.h" and "matrix_stackless.h" (?), but those Matrix_ functions should then still go with SkinMatrix_ functions into "matrix_stackless.h" imo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I am not suggesting one header per c file, but one header per system. These are completely different systems that dont work with each other. Whereas yes, z_actor.c contains a lot of different systems in it, so not all of it will be in the same header.

We gain the granularity to allow the programmer to pick which system they want to include into their file. I dont think you should have to include a bunch of unnecessary things because you want to work with the concept of "matrices" in general.

--

Sure, we could look into some scenario where the matrix stack operations in sys_matrix belong to a new system, and you would have functions like MatrixStack_Init, MatrixStack_Push, MatrixStack_Pop... etc. This would just be a huge improvement anyway, and let the user know point blank that they are messing with the stack.

In this scenario we could also look into changing the SkinMatrix_ system to just Matrix_, along with the other generic matrix operations you mention in sys_matrix.c. This aligns with how things are used anyway-- skin matrix functions clearly started out as helpers for the skin skeleton system, but other devs found it useful outside of that system. It makes sense for us to name it generically to suit that.

Copy link
Collaborator

@fig02 fig02 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Though then we will have to resolve overlaps such as Matrix_MtxFToMtx and SkinMatrix_MtxFToNewMtx. it may get messy 😅. But this is kinda my point of them being seperate systems with their own implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it should be sys_matrix.h and skin_matrix.h. To it seems like "matrix.h" and "matrix_stack.h" is the same thing, except with much worse names, because the names don't match the subsystems they correspond to

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it's evidence but not the main reason, if the two files worked better with each other (or other factors like both being named sys_ or z_) then I would consider them more the same system. The main reason is that the code does not treat sys_matrix and skin_matrix as a unified whole.

For example, Matrix_TranslateRotateZYX and SkinMatrix_SetTranslateRotateZYX theoretically do the same thing, but actually are pretty different. Not just matrix stack vs no stack, but also different parameter types and different implementations that share no code and probably have different rounding behavior (though I haven't checked). So I think it would be confusing to mix them too closely

Copy link
Contributor

@cadmic cadmic Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Now if the goal isn't usability, what is it?

Documentation and reverse-engineering I guess? Like I want to give readers the impression of "oh huh, there's two different matrix subsystems and they're often completely different, that's cool/horrifying/good-to-know".

I don't think it's less usable, if you're modding then you should probably just use sys_matrix and forget about skin_matrix entirely

Copy link
Collaborator

Choose a reason for hiding this comment

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

Think im personally bowing out of this conversation for now, cause I'd just be repeating myself. I've said what I want to say.
Would love to hear more thoughts than just us 3, if anyone else wants to give their opinion.

And lastly I'll say that if we don't come to a conclusion on this now, that we should go back to 1 header until we do decide

Copy link
Collaborator Author

@Dragorn421 Dragorn421 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Documentation and reverse-engineering I guess? Like I want to give readers the impression of "oh huh, there's two different matrix subsystems and they're often completely different, that's cool/horrifying/good-to-know\

Alright that's reasonable to me (though I dislike splitting the systems based on the .c. During development I could see a dev adding a matrix function they need to either file, whichever they come across first)
I'll switch to sys_matrix.h + skin_matrix.h

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I prefer if the headers were separated by their systems.
In this specific case the systems would be named after their C files, yes. That's because it happens to be organized this way. But this doesn't apply to most systems, like all the different systems present on z_actor.c.

So I agree with you dragorn that we should split headers by system, this just happens to more or less match the corresponding C files. Since there's currently no consensus on how to name the headers I think we should name them after the C files since that's the most obvious way, we can always rename them later.

Since you are concerned about usability then you could add a comment at the top of the header explaining the system a bit and how it differentiates to the other matrix header.

tl;dr: I prefer sys_matrix.h + skin_matrix.h and docs comment on each of them.

f32 translateX, f32 translateY, f32 translateZ);
void SkinMatrix_SetTranslateRotateZYX(MtxF* dest, s16 rotX, s16 rotY, s16 rotZ, f32 translateX, f32 translateY,
f32 translateZ);
Mtx* SkinMatrix_MtxFToNewMtx(struct GraphicsContext* gfxCtx, MtxF* src);
void SkinMatrix_SetRotateAxis(MtxF* mf, s16 angle, f32 axisX, f32 axisY, f32 axisZ);

#endif
51 changes: 51 additions & 0 deletions include/matrix_stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#ifndef MATRIX_STACK_H
#define MATRIX_STACK_H

#include "ultra64.h"
#include "z64math.h"

struct GameState;
struct GraphicsContext;

typedef enum {
/* 0 */ MTXMODE_NEW, // generates a new matrix
/* 1 */ MTXMODE_APPLY // applies transformation to the current matrix
} MatrixMode;

void Matrix_Init(struct GameState* gameState);
void Matrix_Push(void);
void Matrix_Pop(void);
void Matrix_Get(MtxF* dest);
void Matrix_Put(MtxF* src);
void Matrix_Mult(MtxF* mf, u8 mode);
void Matrix_Translate(f32 x, f32 y, f32 z, u8 mode);
void Matrix_Scale(f32 x, f32 y, f32 z, u8 mode);
void Matrix_RotateX(f32 x, u8 mode);
void Matrix_RotateY(f32 y, u8 mode);
void Matrix_RotateZ(f32 z, u8 mode);
void Matrix_RotateZYX(s16 x, s16 y, s16 z, u8 mode);
void Matrix_TranslateRotateZYX(Vec3f* translation, Vec3s* rotation);
void Matrix_SetTranslateRotateYXZ(f32 translateX, f32 translateY, f32 translateZ, Vec3s* rot);
void Matrix_MultVec3f(Vec3f* src, Vec3f* dest);
void Matrix_ReplaceRotation(MtxF* mf);
void Matrix_RotateAxis(f32 angle, Vec3f* axis, u8 mode);

#if OOT_DEBUG

Mtx* Matrix_ToMtx(Mtx* dest, const char* file, int line);
#define MATRIX_TO_MTX(gfxCtx, file, line) Matrix_ToMtx(gfxCtx, file, line)

Mtx* Matrix_NewMtx(struct GraphicsContext* gfxCtx, const char* file, int line);
#define MATRIX_NEW(gfxCtx, file, line) Matrix_NewMtx(gfxCtx, file, line)

#else

Mtx* Matrix_ToMtx(Mtx* dest);
#define MATRIX_TO_MTX(gfxCtx, file, line) Matrix_ToMtx(gfxCtx)

Mtx* Matrix_NewMtx(struct GraphicsContext* gfxCtx);
#define MATRIX_NEW(gfxCtx, file, line) Matrix_NewMtx(gfxCtx)

#endif

#endif
2 changes: 0 additions & 2 deletions include/variables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ extern u8 gBossMarkState;

extern s32 gScreenWidth;
extern s32 gScreenHeight;
extern Mtx gMtxClear;
extern MtxF gMtxFClear;
#if OOT_DEBUG
extern u32 gIsCtrlr2Valid;
#endif
Expand Down
7 changes: 2 additions & 5 deletions include/z64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
#include "sys_math.h"
#include "sys_math3d.h"
#include "fp_math.h"
#include "matrix.h"
#include "matrix_stack.h"

#define SCREEN_WIDTH 320
#define SCREEN_HEIGHT 240
Expand Down Expand Up @@ -400,11 +402,6 @@ typedef struct DebugDispObject {
/* 0x28 */ struct DebugDispObject* next;
} DebugDispObject; // size = 0x2C

typedef enum {
/* 0 */ MTXMODE_NEW, // generates a new matrix
/* 1 */ MTXMODE_APPLY // applies transformation to the current matrix
} MatrixMode;

typedef struct StackEntry {
/* 0x00 */ struct StackEntry* next;
/* 0x04 */ struct StackEntry* prev;
Expand Down
2 changes: 2 additions & 0 deletions src/code/sys_matrix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include "global.h"
#include "matrix_stack.h"
#include "matrix.h"

// clang-format off
Mtx gMtxClear = gdSPDefMtx(
Expand Down
1 change: 1 addition & 0 deletions src/code/z_skin_matrix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "global.h"
#include "terminal.h"
#include "matrix.h"

// clang-format off
MtxF sMtxFClear = {
Expand Down
2 changes: 1 addition & 1 deletion src/overlays/actors/ovl_Fishing/z_fishing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include "terminal.h"

// For retail BSS ordering, the block number of sStreamSfxProjectedPos must be 0.
#pragma increment_block_number 182
#pragma increment_block_number 180

#define FLAGS ACTOR_FLAG_4

Expand Down