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

Decomp and integrate sub_8099360 - sub_809965C. #235

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

deppong
Copy link
Contributor

@deppong deppong commented Oct 21, 2024

Quite a few functions so please take the time to go over and feel free to nitpick! I'd be happy to amend any new changes, and It's possible I've overlooked something!

script_disc = GetScriptVarValue(NULL, DUNGEON_ENTER);
if (script_disc == 0x50) {
script_disc = GetScriptVarValue(NULL, DUNGEON_ENTER_INDEX);
*(u8 *)param = script_disc;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here, why volatile and then another cast? Probably unneeded, but if it is needed, it deserves comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess my reasoning is because in src/post_office_guide2.c in DisplayMissionObjectives() a volatile gets passed in, sub_8099394 gets a volatile u8 local_94 passed in, but everywhere else it's called it uses just a regular u8*, so this way it preserves the properties of both. I'm not sure why DisplayMissionObjectives() uses a volatile there, but I didn't want to disrupt the functionality of the function there. Removing the volatile and cast gives the following warning/error:

src/post_office_guide2.c: In function `DisplayMissionObjectives': src/post_office_guide2.c:116: warning: passing arg 1 of `sub_8099394' discards qualifiers from pointer target type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Function still matches without the volatile. Usually we use it for variables to force it on the stack. Might want to try removing volatile from the function declaration in post_office_guide2.c and see if it still matches.

https://decomp.me/scratch/6BPvc <- decomp.me link for sub_8099394

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://github.com/pret/pmd-red/blob/9e4958462189b8dad07f32e1c01931cacf349687/src/post_office_guide2.c#L98C1-L118C45

Ah I actually got it! If in the code above we change line 116 to sub_8099394((u8 *)&local_94); it compiles and make compare is OK. This resolves the dependency of volatile in sub_8099394. I can push the patch to this in a moment

@deppong
Copy link
Contributor Author

deppong commented Oct 21, 2024

I can also rebase/squash if that's preferred

@AnonymousRandomPerson
Copy link
Collaborator

No need, this project doesn't require that level of Git etiquette.

@deppong
Copy link
Contributor Author

deppong commented Oct 22, 2024

Gotcha! Well let me know if there is anything else that seems wrong, otherwise the 2nd commit should have fixed the issue @Kermalis and @SethBarberee mentioned.

@SethBarberee SethBarberee merged commit e3e15a3 into pret:master Oct 22, 2024
1 check passed
@deppong deppong deleted the sub_8099360-809965C branch October 24, 2024 03:35
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.

4 participants