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

Set up multiversion assets with ZAPD and match gc-eu #1967

Merged
merged 13 commits into from
Jun 24, 2024

Conversation

cadmic
Copy link
Contributor

@cadmic cadmic commented Jun 21, 2024

Assets for each version are now extracted into extracted/VERSION/ instead of using debug rom assets. Although this conflicts with the future new assets system, I think this is a step in the right direction and it unblocks matching other versions.

Major changes:

  • Currently, the XMLs for code and overlay assets have BaseAddress, RangeStart, and RangeEnd attributes which give the virtual address of the asset and its offset in its parent ROM segment. These will change for basically every version even though the assets themselves do not, so I moved this info into config.yml. This is implemented with new ZAPD flags --base-address to override the start virtual address for a file, and flags --start-offset/--end-offset to only operate on some slice of the file (different from RangeStart/RangeEnd in that offsets in the XML are relative to --start-offset). It's a bit hacky because these are file-level properties and an XML technically could contain multiple files (though that never happens for code or overlay assets). I'll upstream the ZAPD changes soon: ZAPD features to support OOT multiversion assets NEstelami/ZAPD#322
  • Some versions will require XML changes, for example MQ vs non-MQ dungeons. For these I made separate XML files (for MQ, suffixed with _mq.xml) where config.yml says which XML file to use for each asset. This is perhaps not ideal, but I think any in-XML config would require huge changes to ZAPD. I don't expect this to be too bad, only 20 or so XMLs will change between versions and the differences are mostly broad changes like NTSC vs PAL, N64 vs GC, and MQ vs non-MQ instead of version-specific differences. (See https://docs.google.com/spreadsheets/d/1OGPLrezIfd9MaHZf5s5j0AuncW6EuGuR1ZCn5ZLlRYo/edit#gid=218491254 and https://discord.com/channels/688807550715560050/688851317593997489/823637077786099753 for prior work on object differences.)

Minor changes:

  • ZAPD will now write e.g. #include "assets/objects/gameplay_keep/deku_stick.i8.inc.c" instead of #include "extracted/VERSION/assets/objects/gameplay_keep/deku_stick.i8.inc.c" when including generated files since the generated files could be in either extracted/VERSION/ or build/VERSION/.
  • sShadowTex in ovl_En_Jsjutan is now a "variable" (like gMtxClear) instead of a ZAPD <Symbol> so we don't have to hardcode the offset. This was the only instance of <Symbol> in the project.
  • ZAPD configs are also now split by versions, although most of it is still shared. The only thing that changes is the addresses of gMtxClear and sShadowTex. Potentially we could generate this from config.yml but I didn't think this was worth it.
  • Build artifacts in assets/ are no longer .gitignored since they interfere with the new build (run git clean -f assets to delete these)
  • ovl_File_Choose.xml renamed to ovl_file_choose.xml to match the ROM segment name
  • make distclean is now version-agnostic

@Dragorn421
Copy link
Collaborator

Dragorn421 commented Jun 21, 2024

Paths comparison between main, this PR and new assets
(if I understand this PR right)

main

  • gameplay_keep.c: assets/objects/gameplay_keep/gameplay_keep.c
  • deku_stick.i8.png: assets/objects/gameplay_keep/deku_stick.i8.png
  • deku_stick.i8.inc.c: build/VERSION/assets/objects/gameplay_keep/deku_stick.i8.inc.c
  • #include deku_stick.i8.inc.c: #include "assets/objects/gameplay_keep/deku_stick.i8.inc.c"

This PR #1967

  • gameplay_keep.c: extracted/VERSION/assets/objects/gameplay_keep/gameplay_keep.c
  • deku_stick.i8.png: extracted/VERSION/assets/objects/gameplay_keep/deku_stick.i8.png
  • deku_stick.i8.inc.c: build/VERSION/assets/objects/gameplay_keep/deku_stick.i8.inc.c
  • #include deku_stick.i8.inc.c: #include "assets/objects/gameplay_keep/deku_stick.i8.inc.c"

new assets v4 #1904

  • gameplay_keep.c: assets/objects/gameplay_keep/gameplay_keep.c
  • deku_stick.i8.png: extracted/VERSION/objects/gameplay_keep/deku_stick.i8.png
  • deku_stick.i8.inc.c: extracted/VERSION/objects/gameplay_keep/deku_stick.i8.inc.c
  • #include deku_stick.i8.inc.c: #include "objects/gameplay_keep/deku_stick.i8.inc.c"

@cadmic
Copy link
Contributor Author

cadmic commented Jun 21, 2024

Yeah, keeping assets in the extracted path made several things simpler, though it is inconsistent with text now. Is there a reason why we can't use extracted/VERSION/assets in the new assets system too? I don't think it's a big deal though, it's easy to change since extracted is fully .gitignored

@Dragorn421
Copy link
Collaborator

Is there a reason why we can't use extracted/VERSION/assets in the new assets system too?

Not really, it's just the history of it that led to it being extracted/VERSION/objects, I'm fine with extracted/VERSION/assets/objects too (and off the top of my head I don't see a problem the tooling would have with it)

@Dragorn421
Copy link
Collaborator

cadmic#3

Copy link
Collaborator

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

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

Very nice PR, addresses a bunch of warts I remember dealing with when starting on the new assets system
As a bonus it leaves assets/ empty for it to be filled later :^)

ZAPD would be better modified upstream then pulled into the repo but idk, whatever?

on reviewing, I'm just going to trust the xmls are correct, and if they're not someone will find out eventually (at least it matches)

@cadmic
Copy link
Contributor Author

cadmic commented Jun 22, 2024

Here's the ZAPD PR: NEstelami/ZAPD#322
I've been talking to @louist103 and it seems likely to make it in upstream. So I'd be fine merging this and then updating the subrepo later once the ZAPD PR is merged

subrepo:
  subdir:   "tools/ZAPD"
  merged:   "0285e11f0"
upstream:
  origin:   "https://github.com/zeldaret/ZAPD.git"
  branch:   "master"
  commit:   "0285e11f0"
git-subrepo:
  version:  "0.4.6"
  origin:   "[email protected]:ingydotnet/git-subrepo.git"
  commit:   "110b9eb"
@cadmic
Copy link
Contributor Author

cadmic commented Jun 22, 2024

nvm, the ZAPD PR is merged so I updated the subrepo here

@Dragorn421 Dragorn421 merged commit 9def6f4 into zeldaret:main Jun 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants