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

Document a bunch of info tables (based on comments mainly) #2290

Closed
wants to merge 14 commits into from

Conversation

Feacur
Copy link
Contributor

@Feacur Feacur commented Nov 9, 2024

general reasoning:

  • it's easier to see actual constants than read comments
  • comments were only in one place, but constants are ubiqutous
    • (some comments are preserved, though; did't want to inflate names)

concerning the dialogues with Gorons @ z_en_go.c:

  • these inf table flags are used to not repeat certain messages
  • so, the main purpose of new name is to convey the fact that they've said something already
  • definitely, naming can be improved
    • say, Ingo offers Link a ride after they've met for the first time
      in that case `INFTABLE_9A` for message `0x2030` "do you want to ride one of my fine horses?"
      might be be something like `INFTABLE_TALKED_TO_INGO_FIRST_TIME`, like it was done for Malon
      
    • but sometimes it's a bit of a hassle
      the purpose of `INFTABLE_10F` for messages
      - `0x3041` "Dad and you destroyed the dragon together, didn't you!"
      - `0x3042` "When I grow up, I want to be a strong man like you"
      is to eliminate repeated opening lines. all's good, but `INFTABLE_TALKED_TO_GORON_FIRST_TIME`
      would be a bad name, because the actor profile is shared between entities. i need to find a
      unique name for each of the cases rather than resort to "the-one-over-there-before-an-event"
      these info is better placed as comments so that LSP would tell you any extended meaning
      
      meanwhile `INFTABLE_TALKED_TO_GORON_MSG_3041` or `INFTABLE_HEARD_MSG_3041` should do, i think
      
      `INFTABLE_EB` might be `INFTABLE_KNOW_ABOUT_BOMB_FLOWERS`, but it can be done later, given the
      change makes a difference
      

one way or another will conflict a bit with #2299 at some info tables. negligible, just for me to keep in mind

Feacur added 11 commits November 9, 2024 13:13
was already documented as comments along matching by
- @krimtonz
- @fig02
commit 2810aa2
was already documented as comments along matching by
- @EllipticEllipsis
- @Roman971
ed4021a
was already documented as comments along EVENTCHKINF creation by
- @Dragorn421
6336df5
was already documented by
- fig02
7927e7b
was already documented by
- mzxrules
- fig02
3fd05c9
was already documented as comments along matching by
- @EllipticEllipsis
- @fig02
9d63626
was already documented as comments along matching by
- @Glank
86c4472
was already documented during matching by
- @shawlucas
- @fig02
7097876
because ae4a9dc constants got renamed
@Feacur
Copy link
Contributor Author

Feacur commented Dec 8, 2024

in the light of more atomic PRs for basically the same flags, better to close this one

(... and create new smaller ones if need arises)

@Feacur Feacur closed this Dec 8, 2024
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.

3 participants