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

Some cleanup/doc on the item icons #1263

Merged
merged 39 commits into from
Dec 13, 2022

Conversation

Dragorn421
Copy link
Collaborator

@Dragorn421 Dragorn421 commented Jun 8, 2022

Main things:

  • gItemIcons in code_80097A00.c: format and some comments
  • defines/macros in z64interface.h to replace magic numbers refering to icon_item_static/icon_item_24_static assets ("item icons" and "quest icons")

Copy link
Contributor

@EllipticEllipsis EllipticEllipsis left a comment

Choose a reason for hiding this comment

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

IMO this is a big improvement to readability, although again it conflicts with the AP macros I've been working on :( . I do have a few quibbles over names.

include/z64item.h Outdated Show resolved Hide resolved
include/z64item.h Outdated Show resolved Hide resolved
src/code/code_80097A00.c Outdated Show resolved Hide resolved
src/code/z_parameter.c Show resolved Hide resolved
src/code/z_parameter.c Outdated Show resolved Hide resolved
include/z64item.h Outdated Show resolved Hide resolved
include/z64item.h Outdated Show resolved Hide resolved
include/z64item.h Outdated Show resolved Hide resolved
Copy link
Contributor

@EllipticEllipsis EllipticEllipsis left a comment

Choose a reason for hiding this comment

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

These are all the other ones that stick out as inconsistent.

src/code/code_80097A00.c Outdated Show resolved Hide resolved
include/z64item.h Outdated Show resolved Hide resolved
src/code/code_80097A00.c Outdated Show resolved Hide resolved
src/code/code_80097A00.c Show resolved Hide resolved
src/code/code_80097A00.c Outdated Show resolved Hide resolved
gQuestIconZoraSapphireTex, // ITEM_SPSTONE_ZORA_SAPPHIRE
gQuestIconStoneOfAgonyTex, // ITEM_STONE_OF_AGONY
gQuestIconGerudosCardTex, // ITEM_GERUDOS_CARD
gQuestIconGoldSkulltulaTex, // ITEM_SKULL_TOKEN
Copy link
Contributor

Choose a reason for hiding this comment

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

One of these should definitely change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I have a dilemma here, because ITEM_SKULL_TOKEN is the token item, but gQuestIconGoldSkulltulaTex is a gold skulltula icon that looks nothing like the token

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can still be an icon for it even if it doesn't look the same. The heart piece indicator doesn't look anything like the ones you pick up, for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note I didn't touch the gHeartPieceIcon*Tex names, they indeed probably shouldn't be called "icon" (any suggestion? as well as for the gItemIcons array),
but the (unused) gQuestIconHeartPieceTex does look like the in-game heart piece

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the heart piece one is a gauge/meter, albeit an unconventional layout.

At the end of the day it does still look like a gold skulltula.

(gQuestIconSpidersSquishedTex)

src/code/code_80097A00.c Outdated Show resolved Hide resolved
gQuestIconDungeonBossKeyTex, // ITEM_DUNGEON_BOSS_KEY
gQuestIconDungeonMapTex, // ITEM_DUNGEON_COMPASS (swapped with map?)
gQuestIconDungeonCompassTex, // ITEM_DUNGEON_MAP
gQuestIconDungeonBossKeyTex, // ITEM_SMALL_KEY
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, it uses boss key for both, or...?

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 can't tell if any of these 4 actually uses this array to get the icon, it doesn't seem like it but idk. At least ITEM_SMALL_KEY I think is never used in pause?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added comments on the three odd item->texture mappings (map, compass, small key)
It's inconsequential for all three (as gItemIcons isn't accessed for those items)

src/code/code_80097A00.c Outdated Show resolved Hide resolved
@Dragorn421
Copy link
Collaborator Author

Dragorn421 commented Jun 9, 2022

I'm stuck on understanding what each biggoron sword/giant knife -related item is

ID name (master) name (this PR) suggested? icon in gItemIcons (master) icon in gItemIcons (this PR)
0x33 ITEM_SWORD_BROKEN ITEM_BROKEN_BGS ITEM_BROKEN_GORONS_SWORD gBrokenBiggoronSwordIconTex gItemIconBrokenBiggoronSwordTex
0x3D ITEM_SWORD_BGS ITEM_SWORD_BIGGORON ok? gBiggoronSwordIconTex gItemIconSwordBiggoronTex
0x55 ITEM_SWORD_KNIFE unchanged ITEM_GIANTS_KNIFE gBrokenGiantsKnifeIconTex gItemIconBrokenGiantsKnifeTex

See the B_BTN_ITEM macro and func_800849EC among other things 🤔

@Dragorn421
Copy link
Collaborator Author

I would also update the GI_ enum but that enum is super weird so idk

@fig02 fig02 added the Needs discussion There is a debate or other required discussion preventing changes from being made label Aug 27, 2022
@Dragorn421 Dragorn421 marked this pull request as ready for review November 16, 2022 20:03
@Dragorn421 Dragorn421 changed the title Some cleanup/doc on the item enum and icons Some cleanup/doc on the item icons Nov 16, 2022
@Dragorn421 Dragorn421 removed the Needs discussion There is a debate or other required discussion preventing changes from being made label Nov 16, 2022
@Dragorn421
Copy link
Collaborator Author

With #1376 having been merged this PR is back in business 🎉

include/z64interface.h Outdated Show resolved Hide resolved
include/regs.h Outdated Show resolved Hide resolved
@@ -99,7 +99,7 @@
#define R_TEXTBOX_HEIGHT YREG(23)
#define R_TEXTBOX_ICON_XPOS YREG(71)
#define R_TEXTBOX_ICON_YPOS YREG(72)
#define R_TEXTBOX_ICON_SIZE YREG(75)
#define R_TEXTBOX_ICON_DIMENSION YREG(75)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably prefer Length over Dimension, but it seems fine as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

possibly, though to me length makes it sound like its only for that one side. while this is trying to convey that its the same number for length and width. but yeah i guess its not that clear either way

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'm keeping "dimension" until someone else says they prefer "length"

@fig02 fig02 merged commit 880e7c9 into zeldaret:master Dec 13, 2022
@Dragorn421 Dragorn421 deleted the doc_item_enum_icons branch December 13, 2022 11:18
louist103 pushed a commit to louist103/oot that referenced this pull request Jan 3, 2023
* Some cleanup/doc on the item enum and icons

* `gItemIconBigBottlePoeTex` -> `gItemIconBottleBigPoeTex`

* Biggoron sword / giant knife items (?)

* `SPSTONE_` -> `SPIRITUAL_` ?

* `gItemIconBrokenBiggoronSwordTex` -> `gItemIconBrokenGoronsSwordTex`

* `gQuestIcon...MedallionTex` -> `gQuestIconMedallion...Tex`

* `ITEM_MAGIC_` -> `ITEM_MAGIC_JAR_`small/big

* `ITEM_BOW_ARROW_` -> `ITEM_BOW_`

* `MASK_BUNNY` -> `MASK_BUNNY_HOOD`

* Update `GID_` enum from `ITEM_` enum

* ITEM/GID`_ARROW_`small/medium/large -> 5/10/30

* Run formatter

* .

* fix regressions and revert bad ideas

* chicken

* obey the newline police and also prevent the range police from intervening

* fixups

* dimensions -> dimension (singular)

* Note on inconsequential oddities about the `gItemIcons` mapping
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.

5 participants