-
Notifications
You must be signed in to change notification settings - Fork 647
Use ARRAY_COUNT for Skelanime_Init(Flex)'s table length argument
#2584
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasnt convinced at first but I guess it makes sense. Even the original name is joint_buf_num, the argument is more about the table than the limb count.
I am not a fan that it adds so much more to type for every SkelAnime_Init, but in a modding context maybe ill just macro the call
|
Why do you prefer to use |
|
The purpose of the argument is to check the size of the arrays, it's not just about passing the right number so the function is happy, so it makes more sense to pass the ARRAY_COUNT |
| } else { | ||
| ASSERT(limbCount == skelAnime->limbCount, "joint_buff_num == this->joint_num", "../z_skelanime.c", 2973); | ||
| ASSERT(tableLength == skelAnime->limbCount, "joint_buff_num == this->joint_num", "../z_skelanime.c", 2973); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to have to disagree with using ARRAY_LENGTH to pass in the limb count, because of this line right here. This assert shows that tableLength should be coupled to limbCount, and not necessarily the array length.
In MM, you have z_en_invadepoh.c which manages multiple different NPC characters with incompatible animated models, all under the ACTOR_EN_INVADEPOH instance. In this case, jointTable and morphTable are sized to the largest character, but will call SkelAnime_InitFlex with the different *_LIMB_MAX values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
their original name is joint_buff_num, aka the length of the joint buffers (the table in question).
However your point about usage in MM is a fair one to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird, because even if that is how it was implemented, the code still doesn't fully verify correctness because it assumes morphTable is the same length as jointTable. For that, it seems better to keep using *_LIMB_MAX because it's at least easier to understand.
EDIT: oh and before I forget, the condition probably should be documented as a bug (tableLength should be >= limbCount)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it seems like generally speaking the joint buffer is the "important" one. Its the only one that gets checked for dynamic allocation, for example.
Ultimately I see both approaches here as valid, I didnt necessarily have a problem with limb_max being used as the value.
Regardless of what gets chosen here I think it may be a good idea to name our "limb" enums more appropriately after the buffer indicies. That is, the fact that it has the root translation information which bumps every index up by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert shows that tableLength should be coupled to limbCount, and not necessarily the array length.
Well yes the array length should be coupled to the limb count, it has to store a limb-count-amount of elements
To me a further hint that this check is meant to check the length of the provided arrays is that 0 is passed for dynamically allocated arrays and the value is not checked then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me a further hint that this check is meant to check the length of the provided arrays is that 0 is passed for dynamically allocated arrays and the value is not checked then
I don't doubt this; just question whether it's reasonable to use that pattern when the assert is "bugged"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work in 98% of calls, for reference the only spots that pass a length == limbCount < ARRAY_COUNT(jointTable) are
- https://github.com/Dragorn421/oot/blob/d50962f22d5228aecfd6d6f3946159b98dd94253/src/overlays/actors/ovl_Boss_Sst/z_boss_sst.c#L345-L351
- https://github.com/Dragorn421/oot/blob/d50962f22d5228aecfd6d6f3946159b98dd94253/src/overlays/actors/ovl_En_Dnt_Nomal/z_en_dnt_nomal.c#L202-L203
- https://github.com/Dragorn421/oot/blob/d50962f22d5228aecfd6d6f3946159b98dd94253/src/overlays/actors/ovl_En_Poh/z_en_poh.c#L935-L936
The last argument to
Skelanime_Init(Flex)is a number that, when tables (jointTable and morphTable) are pre-allocated (typically as part of the actor struct), must match the skeleton's limb count. This check ensures the jointTable and morphTable are adequately sized, hence it makes sense to pass anARRAY_COUNTfor that argument