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 bard music #2053

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Document bard music #2053

merged 5 commits into from
Jan 17, 2025

Conversation

GriffinRichards
Copy link
Member

@GriffinRichards GriffinRichards commented Oct 21, 2024

  • Add missing usage of the constant added in Added define value for bard sound length #2052, and correct a mistaken use of it.
  • Rename BARD_SONG_LENGTH to NUM_BARD_SONG_WORDS to avoid confusion with the length of the song in time.
  • Use EC_WORD_* constants for the remaining bard sound arrays

@GriffinRichards GriffinRichards changed the title Missing bard song limit constants Document bard music Oct 25, 2024
@GriffinRichards
Copy link
Member Author

Went down a rabbit hole, this is now documenting some of the opaque parts of how the Bard NPC's music works, including using the PH_ song constants in the bard music data.

Copy link
Collaborator

@mrgriffin mrgriffin left a comment

Choose a reason for hiding this comment

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

I meant to review this ages ago and then it just totally slipped my mind.

#define USE_NEW_SONG (gSpecialVar_0x8004) is the only change that I'm torn on. But I'm not sure I have a good argument against it, just a vague apprehension about hiding a special var behind a macro because when I see something like StartBardSong(USE_NEW_SONG); I assume USE_NEW_SONG to be a constant.

That said, I'm happy to merge this as-is if you're confident that USE_NEW_SONG is an improvement over writing gSpecialVar_0x8004 :)

@GriffinRichards
Copy link
Member Author

Thanks! I do think there's some value in it (giving it a meaningful name, making it easier to swap out if that var will be used for something else in the script), but I'd rather it not be confusing so I'll remove it. Maybe some day someone could work through the repo and give aliases to these vars in a different, standardized way.

@mrgriffin
Copy link
Collaborator

Maybe some day someone could work through the repo and give aliases to these vars in a different, standardized way.

That person would be my hero ✨

@mrgriffin mrgriffin merged commit 2f67b17 into pret:master Jan 17, 2025
1 check passed
github-actions bot pushed a commit that referenced this pull request Jan 17, 2025
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.

2 participants