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

Some Mobile code labeling and removed magic numbers #993

Merged
merged 22 commits into from
Sep 15, 2022
Merged

Some Mobile code labeling and removed magic numbers #993

merged 22 commits into from
Sep 15, 2022

Conversation

ariscop
Copy link
Contributor

@ariscop ariscop commented Aug 30, 2022

Leaves almost no wram0 references which arent labeled
mobile_46 being an exception, c608 has several layouts and needs a union defined

mid-kid
mid-kid requested changes Aug 30, 2022
View reviewed changes
mobile/mobile_46.asm Outdated Show resolved Hide resolved
mobile/mobile_46.asm Outdated Show resolved Hide resolved
mobile/mobile_46.asm Outdated Show resolved Hide resolved
vulcandth
vulcandth requested changes Aug 30, 2022
View reviewed changes
ram/wram.asm Outdated Show resolved Hide resolved
@Rangi42 Rangi42 marked this pull request as draft August 30, 2022 18:51
Rangi42
Rangi42 reviewed Aug 31, 2022
View reviewed changes
ram/wram.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Aug 31, 2022
View reviewed changes
mobile/mobile_46.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Aug 31, 2022
View reviewed changes
mobile/mobile_46.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Aug 31, 2022
View reviewed changes
mobile/mobile_46.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Aug 31, 2022
View reviewed changes
mobile/mobile_46.asm Outdated Show resolved Hide resolved
@ariscop ariscop requested review from vulcandth and mid-kid and removed request for vulcandth and mid-kid September 1, 2022 06:54
@ariscop
Copy link
Contributor Author

ariscop commented Sep 1, 2022

This will probably have to wait on #998
PR's getting larger than i intended it tbh

@mid-kid
Copy link
Member

mid-kid commented Sep 1, 2022

@ariscop I don't think the order matters much but I prefer merging larger PRs first since the smaller are easier to rebase.

vulcandth
vulcandth reviewed Sep 1, 2022
View reviewed changes
ram/sram.asm Outdated
Comment on lines 288 to 299
sOfferEmail:: ds $1e
sOfferTrainerID:: dw
sOfferSecretID:: dw
sOfferGender:: db
sOfferSpecies:: db
sOfferReqGender:: db
sOfferReqSpecies:: db
sOfferMonSender:: ds NAME_LENGTH_JAPANESE - 1
sOfferMon:: party_struct sOfferMon
sOfferMonOT:: ds NAME_LENGTH_JAPANESE - 1
sOfferMonNick:: ds NAME_LENGTH_JAPANESE - 1
sOfferMonMail:: mailmsg_jp sOfferMonMail
Copy link
Collaborator

@vulcandth vulcandth Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just a suggestion; but we could do.

Suggested change
sOfferEmail:: ds $1e
sOfferTrainerID:: dw
sOfferSecretID:: dw
sOfferGender:: db
sOfferSpecies:: db
sOfferReqGender:: db
sOfferReqSpecies:: db
sOfferMonSender:: ds NAME_LENGTH_JAPANESE - 1
sOfferMon:: party_struct sOfferMon
sOfferMonOT:: ds NAME_LENGTH_JAPANESE - 1
sOfferMonNick:: ds NAME_LENGTH_JAPANESE - 1
sOfferMonMail:: mailmsg_jp sOfferMonMail
sMobileOffer:: mobile_offer_struct sMobileOffer

And then put this in macros/wram.asm: (Soon to be renamed to macros/ram.asm)

MACRO mobile_offer_struct
\1Email:: ds $1e
\1TrainerID:: dw
\1SecretID:: dw
\1Gender:: db
\1Species:: db
\1ReqGender:: db
\1ReqSpecies:: db
\1MonSender:: ds NAME_LENGTH_JAPANESE - 1
\1Mon:: party_struct \1Mon
\1MonOT:: ds NAME_LENGTH_JAPANESE - 1
\1MonNick:: ds NAME_LENGTH_JAPANESE - 1
\1MonMail:: mailmsg_jp \1MonMail
ENDM

Likewise for the wOfferMon labels in wram.asm:

wMobileOffer:: mobile_offer_struct wMobileOfffer

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to turn this into a macro struct eventually... however I'd like to wait until more is disassembled before doing so.

Copy link
Member

Choose a reason for hiding this comment

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

Is this struct instantiated more than once or twice?

Copy link
Collaborator

@vulcandth vulcandth Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The entire struct is done twice; once in wram.asm and once in sram.asm..

However there are two additional smaller variations of the struct as well.. Hence why I thought to wait.

The two complete ones:

wOfferEmail::      ds MOBILE_EMAIL_LENGTH
wOfferTrainerID::  dw
wOfferSecretID::   dw
wOfferGender::     db
wOfferSpecies::    db
wOfferReqGender::  db
wOfferReqSpecies:: db
wOfferMonSender::  ds NAME_LENGTH_JAPANESE - 1
wOfferMon::        party_struct wOfferMon
wOfferMonOT::      ds NAME_LENGTH_JAPANESE - 1
wOfferMonNick::    ds NAME_LENGTH_JAPANESE - 1
wOfferMonMail::    mailmsg_jp wOfferMonMail
sOfferEmail::      ds MOBILE_EMAIL_LENGTH
sOfferTrainerID::  dw
sOfferSecretID::   dw
sOfferGender::     db
sOfferSpecies::    db
sOfferReqGender::  db
sOfferReqSpecies:: db
sOfferMonSender::  ds NAME_LENGTH_JAPANESE - 1
sOfferMon::        party_struct sOfferMon
sOfferMonOT::      ds NAME_LENGTH_JAPANESE - 1
sOfferMonNick::    ds NAME_LENGTH_JAPANESE - 1
sOfferMonMail::    mailmsg_jp sOfferMonMail

And the two variations:

wMobileMonSender:: ds NAME_LENGTH_JAPANESE - 1
wMobileMon::       party_struct wMobileMon
wMobileMonOT::     ds NAME_LENGTH_JAPANESE - 1
wMobileMonNick::   ds NAME_LENGTH_JAPANESE - 1
wMobileMonMail::   mailmsg_jp wMobileMonMail
wUnknownGender::     db
wUnknownSpecies::    db
wUnknownReqGender::  db
wUnknownReqSpecies:: db
wUnknownMonSender::  ds NAME_LENGTH_JAPANESE - 1
wUnknownMon::        party_struct wUnknownMon
wUnknownMonOT::      ds NAME_LENGTH_JAPANESE - 1
wUnknownMonNick::    ds NAME_LENGTH_JAPANESE - 1
wUnknownMonMail::    mailmsg_jp wUnknownMonMail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://archives.glitchcity.info/forums/board-76/thread-7509/page-0.html
scroll to PokéCom Center Trade Corner for some context

vulcandth
vulcandth requested changes Sep 1, 2022
View reviewed changes
Copy link
Collaborator

@vulcandth vulcandth left a comment

Choose a reason for hiding this comment

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

Curious, why the char + 1's?

lib/mobile/main.asm Show resolved Hide resolved
lib/mobile/main.asm Show resolved Hide resolved
lib/mobile/main.asm Show resolved Hide resolved
lib/mobile/main.asm Show resolved Hide resolved
@vulcandth vulcandth marked this pull request as ready for review September 8, 2022 00:33
@vulcandth
Copy link
Collaborator

@ariscop stated they were done adding new stuff to the PR. Marking for review. Will review soonish.

@mid-kid mid-kid mentioned this pull request Sep 8, 2022
Closed
mid-kid
mid-kid reviewed Sep 12, 2022
View reviewed changes
macros/ram.asm Outdated Show resolved Hide resolved
@vulcandth vulcandth self-assigned this Sep 12, 2022
vulcandth
vulcandth approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@vulcandth vulcandth left a comment

Choose a reason for hiding this comment

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

I think this is fine as is now. The mobile code overall is a work in progress, and will take many more PRs to label properly.

Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
constants/mobile_constants.asm
Comment on lines +66 to +68
; DION addr $1e + request $8 + Name $5
; + party struct $30 + OT $5 + NICK $5
; + JP Mail struct $2a
Copy link
Member

Choose a reason for hiding this comment

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

Should these be constants?

Copy link
Collaborator

@vulcandth vulcandth Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

They go along with the struct mentioned above... So, ultimately yes. However, I'm still deciding how I want to format the struct. Unless, you just want to make them individual constants for now.

Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
Rangi42
Rangi42 reviewed Sep 13, 2022
View reviewed changes
mobile/mobile_5f.asm Outdated Show resolved Hide resolved
@vulcandth @Rangi42
Apply Rangi Suggestions pass 1
8ed76eb 
Co-Authored-By: Rangi <35663410+Rangi42@users.noreply.github.com>
mid-kid
mid-kid reviewed Sep 13, 2022
View reviewed changes
lib/mobile/main.asm Outdated Show resolved Hide resolved
@vulcandth vulcandth changed the title Many things labeled and magic numbers removed Some Mobile code labeling and removed magic numbers Sep 15, 2022
@vulcandth vulcandth merged commit 0f55407 into pret:master Sep 15, 2022
github-actions bot pushed a commit that referenced this pull request Sep 15, 2022
@vulcandth @Rangi42
Some Mobile code labeling and removed magic numbers (#993)
40b5481 
* Macro loop for EmptyAllSRAMBanks

* Many things labeled and magic numbers removed

* Conform to the style guide

* Rename URLs in mobile_46

* MOBILEAPI_19 -> MOBILEAPI_TELEPHONESTATUS

* Update engine/menus/empty_sram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update ram/wram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Remove underscore from Set*DownloadURL symbols

* Begin Labeling TradeCornerHoldMon routines

* Add wMobileMon party_struct

* Add TRADE_CORNER_REQUEST_LENGTH constant

* Name battle tower action functions

* Label trade corner send/receive buffer

* Label the saved copy of the trade corner pokemon

also fix mailmsg_jp

* So many copies of decodeBase64Character

* Label another buffer

* Fix Alignment and use MOBILE_EMAIL_LENGTH

* `Function11ad8a` -> `MobileIncJumptableIndex`

* Apply Rangi Suggestions pass 1

Co-Authored-By: Rangi <35663410+Rangi42@users.noreply.github.com>

* Apply Suggestions 2

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>
Co-authored-by: vulcandth <vulcandth@gmail.com>
Idain pushed a commit to Idain/pokecrystal that referenced this pull request Sep 11, 2023
@ariscop @Rangi42
@vulcandth @Idain
Some Mobile code labeling and removed magic numbers (pret#993)
8d6b99f 
* Macro loop for EmptyAllSRAMBanks

* Many things labeled and magic numbers removed

* Conform to the style guide

* Rename URLs in mobile_46

* MOBILEAPI_19 -> MOBILEAPI_TELEPHONESTATUS

* Update engine/menus/empty_sram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update ram/wram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Remove underscore from Set*DownloadURL symbols

* Begin Labeling TradeCornerHoldMon routines

* Add wMobileMon party_struct

* Add TRADE_CORNER_REQUEST_LENGTH constant

* Name battle tower action functions

* Label trade corner send/receive buffer

* Label the saved copy of the trade corner pokemon

also fix mailmsg_jp

* So many copies of decodeBase64Character

* Label another buffer

* Fix Alignment and use MOBILE_EMAIL_LENGTH

* `Function11ad8a` -> `MobileIncJumptableIndex`

* Apply Rangi Suggestions pass 1

Co-Authored-By: Rangi <35663410+Rangi42@users.noreply.github.com>

* Apply Suggestions 2

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>
Co-authored-by: vulcandth <vulcandth@gmail.com>
Idain pushed a commit to Idain/pokecrystal that referenced this pull request Sep 12, 2023
@ariscop @Rangi42
@vulcandth @Idain
Some Mobile code labeling and removed magic numbers (pret#993)
d025298 
* Macro loop for EmptyAllSRAMBanks

* Many things labeled and magic numbers removed

* Conform to the style guide

* Rename URLs in mobile_46

* MOBILEAPI_19 -> MOBILEAPI_TELEPHONESTATUS

* Update engine/menus/empty_sram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update ram/wram.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Update mobile/mobile_46.asm

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

* Remove underscore from Set*DownloadURL symbols

* Begin Labeling TradeCornerHoldMon routines

* Add wMobileMon party_struct

* Add TRADE_CORNER_REQUEST_LENGTH constant

* Name battle tower action functions

* Label trade corner send/receive buffer

* Label the saved copy of the trade corner pokemon

also fix mailmsg_jp

* So many copies of decodeBase64Character

* Label another buffer

* Fix Alignment and use MOBILE_EMAIL_LENGTH

* `Function11ad8a` -> `MobileIncJumptableIndex`

* Apply Rangi Suggestions pass 1

Co-Authored-By: Rangi <35663410+Rangi42@users.noreply.github.com>

* Apply Suggestions 2

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>
Co-authored-by: vulcandth <vulcandth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vulcandth vulcandth vulcandth approved these changes

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

@Rangi42 Rangi42 Rangi42 approved these changes

Assignees

@vulcandth vulcandth

Labels
renaming
Projects
None yet
Milestone
No milestone
Development

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