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

Improve Pyramid location documentaion #2073

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AsparagusEduardo
Copy link
Contributor

Description

The InBattlePyramid name was a bit of a misnomer, as it could return 0, 1 or 2 instead of just 0 and 1.
Added an in-line function with the original name to reduce verboseness and potential downstream conflicts.

Discord contact info

AsparagusEduardo

@SBird1337
Copy link
Collaborator

hmm, actually with the new semantics of InBattlePyramid if someone had

if (InBattlePyramid() == 2)

they'll get surprising results. I'd personally rather have an error pointing me to the new name.

@AsparagusEduardo
Copy link
Contributor Author

Good point.
I'll change the checks for if (InBattlePyramid()) and such to if (CurrentBattlePyramidLocation() != PYRAMID_LOCATION_NONE)

@AsparagusEduardo
Copy link
Contributor Author

Applied :)

Comment on lines +93 to +95
if (CurrentBattlePyramidLocation() != PYRAMID_LOCATION_NONE || InBattlePike() || InTrainerHillChallenge())
#else
if (InBattlePyramid() | InBattlePike() || InTrainerHillChallenge())
if (CurrentBattlePyramidLocation() | InBattlePike() || InTrainerHillChallenge())
Copy link
Member

@GriffinRichards GriffinRichards Dec 7, 2024

Choose a reason for hiding this comment

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

These should be the same, otherwise it looks like the difference is significant for the bug fix (I guess in some sense it is, but then it's much easier to miss the | vs ||)

if (!InBattlePyramid())
if (CurrentBattlePyramidLocation() == PYRAMID_LOCATION_NONE) // CurrentBattlePyramidLocation() == PYRAMID_LOCATION_NONE doesn't match
Copy link
Member

Choose a reason for hiding this comment

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

Comment left behind

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.

3 participants