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.

Sign up for GitHub

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

Jump to bottom

Remove enum; add const_skip and const_next #740

Merged
merged 2 commits into from
Jul 11, 2020
Merged

Remove enum; add const_skip and const_next #740

merged 2 commits into from
Jul 11, 2020

Conversation

Rangi42
Copy link
Member

@Rangi42 Rangi42 commented Jul 8, 2020 •
edited
Loading

Corresponds to pret/pokegold#55

@Rangi42 Rangi42 requested a review from mid-kid July 8, 2020 20:21
mid-kid
mid-kid requested changes Jul 8, 2020
View reviewed changes
Copy link
Member

@mid-kid mid-kid left a comment

Choose a reason for hiding this comment

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

Oh boy do I have the barrel of nitpicks for ya.

constants/deco_constants.asm Show resolved Hide resolved
constants/event_flags.asm Outdated Show resolved Hide resolved
constants/event_flags.asm Outdated Show resolved Hide resolved
constants/map_constants.asm Show resolved Hide resolved
constants/trainer_constants.asm Show resolved Hide resolved
macros/scripts/audio.asm Outdated Show resolved Hide resolved
macros/scripts/battle_commands.asm Outdated Show resolved Hide resolved
macros/scripts/gfx_anims.asm Outdated Show resolved Hide resolved
macros/scripts/movement.asm Outdated Show resolved Hide resolved
wram.asm
; 0 nightmare
; 1 curse
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be handled in a different PR, but why document the bits here when they're documented in constants/ and associated with the proper label in a comment? Maybe this is one of those places where EXPORT is a good idea?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'd rather just delete those redundant comments in wram.asm. The SUBSTATUS_* constant definitions already comment that they apply to wPlayerSubStatus1, wPlayerSubStatus2, etc, and have descriptive names. Maybe this wram.asm comment could just be "; takes a SUBSTATUS_* value`".

(Also, definitely save that for a separate PR, because I think the constants should also be SUBSTATUS1_*, SUBSTATUS2_*, etc, to make it clear which go where.)

@Rangi42 Rangi42 requested a review from mid-kid July 9, 2020 14:58
@Rangi42 Rangi42 force-pushed the master branch 2 times, most recently from 35faf00 to fcf041a Compare July 9, 2020 16:15
This was referenced Jul 9, 2020
Merged
Merged
mid-kid
mid-kid approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@mid-kid mid-kid left a comment

Choose a reason for hiding this comment

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

Stuff left for a different PR:

  • Remove bit descriptions from wram.asm
  • Use RSSET for structures, instead of the ugly const_skip for the higher byte currently.

constants/pokemon_constants.asm Show resolved Hide resolved
@Rangi42 Rangi42 merged commit 4fb0088 into pret:master Jul 11, 2020
@Rangi42 Rangi42 mentioned this pull request Jul 11, 2020
Closed
pokepret pushed a commit that referenced this pull request Jul 11, 2020
Merge pull request #740 from Rangi42/master
e4bb63b 
Remove enum; add const_skip and const_next
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mid-kid mid-kid mid-kid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rangi42 @mid-kid