convert palettes.asm to standard hex colors #465
Conversation
| @@ -1370,712 +1370,718 @@ func_021_54F9:: | |||
| pop de ; $5516: $D1 | |||
| ret ; $5517: $C9 | |||
|
|
|||
|
|
|||
| ; These 6 object palettes are always in RAM while you're controlling Link | |||
|
|
|||
| Data_021_5518:: | |||
There was a problem hiding this comment.
Perhaps rename this label, and the following ones with comments that indicate you've discovered what they're used for? For example
| Data_021_5518:: | |
| GreenObjectPalette:: |
There was a problem hiding this comment.
Yes, renaming the most obvious labels could be a nice addition, if you still have some bandwidth for this.
(But otherwise for me this PR already stands by itself: the documentation pass can also come later.)
There was a problem hiding this comment.
ah, I wasn't sure whether they should be labeled by their identity, or by their usage. for example, I'm not sure if that label is intended to reference the green object palette specifically, or the group of 6 object palettes.
There was a problem hiding this comment.
depending on the code, it might make sense to have two labels there:
ObjectPalettes::
GreenObjectPalette::
There was a problem hiding this comment.
and with the red and blue tunic, I wasn't sure if they should be labeled RedObjectPalette (what the palette is for) or RedTunicPalette (what the label is for).
There was a problem hiding this comment.
Two labels are definitely a possibility (this pattern is used several times in the code).
About the type of the label, hmm, I think RedTunicPalette would be more helpful for people browsing through the code: instead of having to read the code at the call site to see a comment stating "this red palette is used for the red tunic", one can just read the data. But it's definitely open to discussions :)
There was a problem hiding this comment.
committing this as-is for now, and I'll change the labels when I figure out what the green label is used for.
Closes #356! Thank you to @Calindro and @daid for help with this.