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

Port Proposed OoT Object Docs #1361

Merged
merged 5 commits into from
Oct 4, 2023
Merged

Port Proposed OoT Object Docs #1361

merged 5 commits into from
Oct 4, 2023

Conversation

fig02
Copy link
Contributor

@fig02 fig02 commented Aug 22, 2023

These docs port the proposed terminology by @Dragorn421 that has been waiting in a PR in OoT for quite a while now: zeldaret/oot#1227

By opening this PR for MM I hope to get more eyes on these proposed changes to further discussions, and try to keep the terminology in sync for both games.

This PR focuses only on changes within z_scene. There are many places in the codebase that use the "objBankIndex" term. To keep the PR palatable to review, I opted to leave that for another cleanup pass.

In OoT, the main point of discussion holding the PR up is what to call Object_InitBank. I opted to use Object_InitContext in this PR, but there are arguments against using this name. I encourage anyone reviewing this to read the whole discussion here: zeldaret/oot#1227 (comment)

@AngheloAlf AngheloAlf added documentation Improvements or additions to documentation Needs-second-approval Second approval Needs-first-approval First approval labels Aug 22, 2023
@hensldm
Copy link
Collaborator

hensldm commented Aug 29, 2023

Fwiw, I've already approved the OoT version and I still agree with it over here, but will refrain from approving this one until we get more eyes on it.

Copy link
Contributor

@isghj5 isghj5 left a comment

Choose a reason for hiding this comment

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

Looks good to me, the only thing that bothers me is the loss of the object area sizes define, which feels like a regression but I can't think of a good reason for those to exist after all.

@fig02
Copy link
Contributor Author

fig02 commented Sep 5, 2023

Yeah they dont accomplish anything. The constants get used in one place, and its more helpful to see the value straight away instead of needing to open a header.

Copy link
Collaborator

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

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

As with OoT, the changes look good.

Could you add Object_GetSlot, Object_SpawnPersistent and play->objectCtx.slots to namefixer.py. Those functions/member seem to be the only ones that are used outside the system itself

Copy link
Collaborator

@engineer124 engineer124 left a comment

Choose a reason for hiding this comment

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

These changes are fine with me, just have to put the docs in namefixer and make jenkins happy

@engineer124 engineer124 removed the Needs-first-approval First approval label Sep 24, 2023
@AngheloAlf AngheloAlf added Merge-ready All reviewers satisfied, just waiting for CI Waiting-for-author Author needs fix to conflicts or address reviews and removed Needs-second-approval Second approval labels Sep 24, 2023
@engineer124
Copy link
Collaborator

Got permission from Fig to resolve conflicts. Also added changes to namefixer.py. Will merge once jenkins is happy

@engineer124 engineer124 removed the Waiting-for-author Author needs fix to conflicts or address reviews label Oct 4, 2023
@engineer124 engineer124 merged commit f4a490b into zeldaret:main Oct 4, 2023
@engineer124 engineer124 mentioned this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Merge-ready All reviewers satisfied, just waiting for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants