Skip to content

d_a_npc_jagar - ~94%#2309

Merged
hatal175 merged 47 commits intozeldaret:mainfrom
carter-ktb21:main
Mar 7, 2025
Merged

d_a_npc_jagar - ~94%#2309
hatal175 merged 47 commits intozeldaret:mainfrom
carter-ktb21:main

Conversation

@carter-ktb21
Copy link
Copy Markdown
Contributor

@carter-ktb21 carter-ktb21 commented Feb 24, 2025

Jagar class almost equivalent.

@carter-ktb21 carter-ktb21 marked this pull request as draft February 24, 2025 02:45
@carter-ktb21 carter-ktb21 changed the title d_a_npc_jagar - ~78% d_a_npc_jagar - ~94% Mar 2, 2025
@carter-ktb21 carter-ktb21 marked this pull request as ready for review March 3, 2025 01:58
Copy link
Copy Markdown
Contributor

@TakaRikka TakaRikka left a comment

Choose a reason for hiding this comment

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

overall looks good, just some cleanup that could be done and double check that nonmatching functions are labeled appropriately as such. you do seem to have a build error however that will need to be fixed

@@ -276,33 +280,17 @@ extern "C" extern void* __vt__14cCcD_ShapeAttr[22];
extern "C" extern void* __vt__9cCcD_Stts[8];
extern "C" extern void* __vt__14J3DMaterialAnm[4];
extern "C" u8 now__14mDoMtx_stack_c[48];
extern "C" extern u8 g_meter2_info[248];
extern "C" extern dMeter2Info_c g_meter2_info;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this can be removed since you're already including d_meter2_info.h

Comment on lines +510 to +518
mTwilight = 0; /* WIP */
int rv = loadRes(l_loadResPtrnList[mType], (const char**)l_resNameList);
if (rv == cPhs_COMPLEATE_e) {
// OS_REPORT("\t(%s:%d) flowNo:%d, PathID:%02x<%08x> ", fopAcM_getProcNameString(this), (uint)mType,
// field_0xa7c, getPathID(), fopAcM_GetParam(this));
if (isDelete()) {
return cPhs_ERROR_e;
}
// OS_REPORT("\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are these comments important? if the function is matching i'd assume its fine to uncomment and remove the temp one?

0x00,
0x00,
0x00,
static int const heapSize[4] = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just a note, any data that ends with something like $1234 is an in-function static. so this probably belongs in the create func

Comment on lines +639 to +643
bool rv = true;
if (!daNpcT_chkEvtBit(0xd3) && !dComIfGs_isCollectShield(0)) {
rv = false;
}
return rv;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this work as a single line return?

return daNpcT_chkEvtBit(0xd3) || dComIfGs_isCollectShield(0);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not :/

Comment on lines +654 to +680
csXyz acStack_20;
int iVar1 = (u8*)&field_0x1008 - (u8*)&field_0xfd4;
if (mpMatAnm != NULL) {
mpMatAnm->initialize();
}
initialize();
for (int i = 0; i < 5; i++) {
mActorMngr[i].initialize();
}
memset(&field_0xfd4, 0, iVar1);
acStack_20.setall(0);
acStack_20.y = home.angle.y;
switch (mType) {
case TYPE_0:
if (daNpcT_chkEvtBit(0x1c) != 0) {
if (daNpcT_chkEvtBit(0x86) == 0) {
daNpcT_onEvtBit(0x86);
}
field_0x1001 = 1;
}
case TYPE_1:
case TYPE_2:
default:
daNpcT_offTmpBit(0x1b);
daNpcT_offTmpBit(0x10);
setAngle(acStack_20);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just a general style note that putting new lines after things like if statements, for loops, etc helps functions look more readable. dont worry too much about it, i wouldnt reject over it, but just something to keep in mind

COMPILER_STRIP_GATE(0x80A1A40C, &lit_4932);
#pragma pop
// /* 80A1ADE0-80A1ADE4 000008 0001+03 1/1 0/0 0/0 .bss @1109 */
// static u8 lit_1109[1 + 3 /* padding */];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can try replacing these unused bss literals with the UNK_REL_BSS macro to make it look cleaner until we find a better solution

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I put that macro towards the top. Is the placement important?

void daNpc_Jagar_c::chkToMotion() {
// NONMATCHING
int daNpc_Jagar_c::chkToMotion() {
// The inline assembly was the only way I could find to get proper regalloc
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if this is still nonmatching the nonmatching tag should be kept. otherwise, all the asm comments should probably be removed. its pretty much certain that these were not written with inline asm so it shouldnt be used here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Put nonmatching tags where they should still be

}
if (mHide != 0) {
fopAcM_setCullSizeBox((fopAc_ac_c *)this, -300.0f, -50.0f, -200.0f, 300.0f, 300.0f, 400.0f);
goto LAB_80a18a8c;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should probably put some kind of note about this being a fakematch, as its very unlikely they originally used gotos here

#pragma pop
// /* ############################################################################################## */
// /* 80A1AE4C-80A1AE50 000074 0004+00 0/0 0/0 0/0 .bss
// * sInstance__40JASGlobalInstance<19JASDefaultBankTable> */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these comments should be able to removed now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

hatal175
hatal175 previously approved these changes Mar 6, 2025
@hatal175 hatal175 dismissed their stale review March 6, 2025 22:02

missed build not passing

@hatal175
Copy link
Copy Markdown
Contributor

hatal175 commented Mar 6, 2025

Needs to fix
886: SECTION_RODATA u8 const daNpc_Bou_Param_c::m[156] = {

Error: ^

identifier 'daNpc_Bou_Param_c::m' redeclared

@hatal175 hatal175 merged commit 0f141c2 into zeldaret:main Mar 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants