-
Notifications
You must be signed in to change notification settings - Fork 615
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 Magic #1199
Merged
Merged
Document Magic #1199
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
7782db3
Magic docs WIP
engineer124 b5e6ac8
More docs, first round finished
engineer124 bf2858b
Better docs
engineer124 bdda1e5
More renaming
engineer124 6bd46b2
Simpler name
engineer124 354c594
Another small adjustment
engineer124 e359fce
rm if(1)
engineer124 80de240
Better names again after in-game testing
engineer124 522ee08
Change comments
engineer124 764a102
change comment
engineer124 9125e97
Merge branch 'master' into magic
engineer124 e2eada8
Big rename based on all the suggestions
engineer124 f05f8c2
Merge branch 'master' into magic
engineer124 26adeef
Small touch-up
engineer124 bd3174d
Merge branch 'master' into magic
engineer124 0146061
More PR Suggestions
engineer124 aa10f47
RESTORE_IDLE -> RESET
engineer124 fe133ce
More docs
engineer124 9779f9e
Capitalization
engineer124 727a5bd
PR suggestions
engineer124 5919acc
Make declaration consistent
engineer124 d647ba1
Health_ChangeBy (amount)
engineer124 d992fb5
Merge branch 'master' into magic
engineer124 2ce2812
PR Suggestions
engineer124 e0776f5
Missed one
engineer124 a920403
Merge branch 'master' into magic
engineer124 743bb67
More PR Suggestions
engineer124 ad2fbd9
Change comment
engineer124 ca12b3d
Add another clarity comment
engineer124 6a9468a
Merge branch 'master' into magic
engineer124 b9e8936
Discord discussions on `magicFillTarget`
engineer124 af5d5c9
Comments
engineer124 34a7451
grammar
engineer124 72306ba
More comment clarity
engineer124 c540364
Another bad comment
engineer124 c7225d1
PR suggestions, improved comments
engineer124 a7a0d00
One more comment
engineer124 2bd92ab
one more thing
engineer124 b34a259
bar -> meter
engineer124 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realise this was used extensively in the repo, but now I'm wondering if we want to name variables like this interrogatively... does the
is
actually do anything here, that the plain past participle doesn't?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this specific instance,
doubleMagicAcquired
alone can be inferred to be a boolean-like value (it still uses the 0/1 in calculatingmagicCapacity
so it's not a true bool), but as you mentioned,is
is just a common and fast way to identify that something is boolean without thinking about it, at least that was my understanding of the conventionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as a side note, not related to this comment but something I wanted to note:
doubleDefense
->isDoubleDefenseAcquired
(or without theis
) would be consistent withisDoubleMagicAcquired
but I refrained from changing it for nowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were going by the game it should be "half damage" (or possibly "enhanced defence") anyway:
(I suppose we could say the same thing for magic, actually, it never explicitly says it doubles, although that's obvious from the bar size)
There was a problem hiding this 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 case where the common colloquial name makes more sense. no one says half damage or enhanced defense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look into this, since I was curious. I looked at 4 guidebooks (3 English, 1 the official one, 1 Japanese) and the first page of Google results for walkthroughs, including 4 GameFAQs guides. A large majority say one or more of
and only 2 (of about 20) say double/doubled defense. I was pretty surprised by this myself, since the tiny speedrunning corner of the world does seem to call always call it "double defense", probably because of the alliteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add a different perspective, I come from the randomizer community and never heard of any of those terms, it's only ever been "double defense" for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other related names in decomp:
FAIRY_UPGRADE_HALF_DAMAGE
(hi elliptic),gGreatFairyDoubleDefenceCs
(hi louis)I would be okay with normalizing all of that to "half damage", with a comment on the save context struct member like
// double defense
"double defense" probably is so popular because it rolls so well off the tongue