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

bank03.asm Function Descriptions & Translations #110

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

Conversation

tdadvocate
Copy link

Comments added to many functions and text strings that were not fully disassembled. Also translated to some of the text within the file related to the functions that have been researched.

Comments added to many functions and text strings that were not fully disassembled. Also translated to some of the text within the file related to the functions that have been researched.
@vulcandth
Copy link
Collaborator

Thanks i'll look this over soon.

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.

Comments are not a substitute for function names. Most of these labels can be renamed to the exact contents of your comments. The moment you start copy-pasting comments, it should raise red flags that something is deeply wrong.

engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
Beginning to rename functions/text names to match gold/yellow/red depending on which final game was closest to the code/text in the demo. Also updating/adding some translations.
Continuing to rename functions/text names to match gold/yellow/red depending on which final game was closest to the code/text in the demo. Also updated/added some more translations.
Missed this change in the last commit since I apparently didn't save the file. Whoops. Last commit for this session.
Adding the setup instructions from my personal story mode patch fork so that more new devs can maybe attempt to assist with the disassembly further. This README may change down the road to be more in-line with the main PRET repo to see if they possibly allow me to merge the Build Instructions section to their README to help the project continue to grow.
@vulcandth
Copy link
Collaborator

@tdadvocate Just comment when you are ready for another review.

Adding a hopefully relatively consistent naming scheme for some more func/data/text that was not previously named. Trying to keep consistent with final Gold/Yellow/Red  naming schemes as much as possible. This should hopefully be the final bits of what was necessary for the pull request to the PRET repo.
@tdadvocate
Copy link
Author

So I did this across multiple commits as I had time on my fork. How would I go about requesting a review? This is my first time doing pull requests so I am not sure if it is best for me to close this and request a new pull based on all the commits that I made over the past 2 weeks or if there is some way that I can just merge them all together on this request. Sorry for being a pain and thank you for all of the help so far!

I reverted 2 text name changes as they might be incorrect assumptions based on recent findings while continuing to translate text within the file. I also added a comment or two on some functions that I need to research further in the future when I have more time.
@mid-kid
Copy link
Member

mid-kid commented Nov 14, 2023

Feel free to ping me or people in the discord if you need quicker reviews. I only noticed your fixes right now, and I won't have time to look at it for another few days at least.

@vulcandth
Copy link
Collaborator

vulcandth commented Nov 14, 2023

So I did this across multiple commits as I had time on my fork. How would I go about requesting a review? This is my first time doing pull requests so I am not sure if it is best for me to close this and request a new pull based on all the commits that I made over the past 2 weeks or if there is some way that I can just merge them all together on this request. Sorry for being a pain and thank you for all of the help so far!

My bad; I thought you were still making commits and missed your comment here. It's easier to skip past notifications on GitHub. Within reason of course, you are more than welcome to ping us on discord in the future to let us know you are ready for reviews as mid-kid stated. I'll review this tonight when I get home from work. Again, thank you for contributing!!!

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.

Didn't have time to review all of it; but this should give you some stuff to work on.

As mid-kid pointed out; the labels don't need to be perfect.. We can always go back and refine them later... however they will make reading the code easier even if they are not 100% accurate.

README.md Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
text "を すてます"
line "ほんとに よろしいですか?"
text "を すてます" ; "Are you sure you want"
line "ほんとに よろしいですか?" ; "to throw (item?) away?"
prompt

Textd491:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Textd491:
ItemsTooImportantText:

engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
engine/dumps/bank03.asm Outdated Show resolved Hide resolved
This reverts commit 4e9baa9.
Reverting a few function names that I discovered I named poorly. Will be trying to make more changes based on the feedback received by the team as soon as I can. Just want to get these few reverted for now so that I can properly research them further without reverting all of the other changes I did in a previous commit that were actually helpful.
Merge PRET Master Changes (More Decomp Work)
Making corrections and additional name changes based on the amazing suggestions from vulcandth.
@tdadvocate
Copy link
Author

I believe that all the changes requested should now match up to what was suggested. I skimmed back through and I don't see any immediately noticeable errors from my new understanding of things. If I did miss something, please let me know and I will be more than happy to correct them! Thank you both for all of your help on so far on supporting this project!

@vulcandth
Copy link
Collaborator

This currently fails to build. Make sure you do a make compare and verify it is matching, and it builds before pushing :).

error: engine/dumps/bank03.asm(332):
    'ItemsThrowAwayText' already defined at engine/dumps/bank03.asm(326)

error: engine/dumps/bank03.asm(3413):
    "BallSoCloseText" is not a macro

Textebc3:
text "ざんねん!"
line "もうすこしで つかまえられたのに!"
BallAlmostHadItText:BallSoCloseText
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
BallAlmostHadItText:BallSoCloseText
BallAlmostHadItText:

Here is one of your build errors.

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.

After fixing the build, and finishing out these last few changes.. This looks good enough to merge.

@@ -2566,14 +2566,14 @@ Functione49d:
call PlaceString
ret

Texte4bf:
db "あずかっている#"
Texte4bf: ; unfinished feature to show how many mon are in your box
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Texte4bf: ; unfinished feature to show how many mon are in your box
MonInMyCareText:

Something like this is fine.

next " @"

Texte4ca:
Texte4ca: ; max mon per box
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Texte4ca: ; max mon per box
OutOfThirtyText:

and a
ret

Functione51f:
ld hl, Texte529
BoxChangeUnderDevTextFunc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
BoxChangeUnderDevTextFunc:
PrintBoxChangeUnderDev:

prompt

Functione53e:
ld hl, Texte551
WhenYouChangeBoxTextFunc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
WhenYouChangeBoxTextFunc:
PromptChangeBoxWillYouSave:

done

Functione57e:
BoxNameChangeFunc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
BoxNameChangeFunc:
ChangeBoxName:

Since we append things like Text, RLE, Data, ect at the end of data labels... It doesn't add value to add Func at the end of Routines.

@@ -2760,16 +2760,16 @@ Functione5d3:
ret

Texte679:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Texte679:
CurrentBoxText:

Something like..

@@ -2760,16 +2760,16 @@ Functione5d3:
ret

Texte679:
db "ボックス/いまの ボックス@"
db "ボックス/いまの ボックス@" ; "Box/Current Box (Name)"

Texte687:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Texte687:
SpeciesNameLevelText:


Texte687:
db "しゅるい  なまえ   レべル@"
db "しゅるい  なまえ   レべル@" ; "Species Name Level"

Texte697:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Texte697:
WhichOneWouldYouLikeToSeeText:

This should fix the build error as well as make the suggested corrections from Vulcandth so that it should be compatible. I hope. Lol.
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