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

Clean up custom LoadTextureBlock in z_map_mark.c and z_lmap_mark.c #1896

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mzxrules
Copy link
Contributor

the definitions of gdpLoadTextureBlock and similar use token replacement to construct the constants passed into macro's internal display list calls, meaning the macro cannot be used to pass in raw IM_SIZ values during runtime. To solve this, the original uses a series of six lookup tables to fetch the correct constant values.

To get the original gdpLoadTextureBlock to reference these arrays, decomp currently creates 6 specially named defines for every uniquely named siz argument that needs to be passed into gdpLoadTextureBlock.

There's a few things I find wrong with this:

  • In general, I find it poor design to have to create 6 defines for every different possible argument you'd pass into a macro, and I fear that somebody who is too afraid to look into the gdpLoadTextureBlock internals is going to blindly copypaste the pattern because it just works
  • If a custom mod updates gdpLoadTextureBlock to allow raw IM_SIZ values, the changes won't automatically eliminate the array access since the decomp code still passes in the result of the sBaseImageSizes lookup into the macro.
  • Technically, z_map_mark.c and z_lmap_mark.c is coupled to gdpLoadTextureBlock's internals more than it probably should be.

Copy link
Contributor

@hensldm hensldm left a comment

Choose a reason for hiding this comment

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

Fwiw, I don't mind this solution. Kind of annoying dealing with data that is both in data and in rodata in separate places, but this doesn't seem too bad.

include/gDPLoadTextureBlock_Runtime.inc.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants