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

Add Dockerfile, read raw romfs with zstd+sarc+byml #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aquacluck
Copy link

This changes how build.ts is called, and also fails if these tools aren't installed which might be a little controversial. Runtime is also increased since we'll decompress+decode on every re-import, but thankfully it doesn't seem too excessive to me.

Enables https://github.com/aquacluck/totk-objmap-docker

@aquacluck
Copy link
Author

this effectively addresses #7 too, although I don't have the webp map tiles either

@leoetlino
Copy link
Member

This changes how build.ts is called, and also fails if these tools aren't installed which might be a little controversial. Runtime is also increased since we'll decompress+decode on every re-import, but thankfully it doesn't seem too excessive to me.

Auto-decompress/conversion should be a fallback, not a requirement. It doesn't make sense to force everyone to install additional tools and to decompress/convert over and over again just because some people can't be bothered to decompress/convert once ahead of time :P

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

needs fallback handling

build.ts Outdated
Comment on lines 26 to 28
const zsDicPath = fs.mkdtempSync('zsdicpack');
execSync(`zstd -d "${romfsPath}/Pack/ZsDic.pack.zs" -o "${zsDicPath}/ZsDic.pack"`);
execSync(`sarc x --directory "${zsDicPath}" "${zsDicPath}/ZsDic.pack"`);
Copy link
Author

@aquacluck aquacluck Jul 9, 2023

Choose a reason for hiding this comment

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

I think these are the only potentially breaking calls for people who've already unpacked files. If we only do this when we need it, that should all be compatible, just give it a romfs-looking folder with the unpacked files.

I think that works for everyone? To be clear this PR already accepts unpacked files, it doesn't force you to re extract everything if you've already done it... unless you leave eg byml+yml files coexisting in the same folder, but we could reorder those ifs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not have a romfs

Copy link
Author

@aquacluck aquacluck Jul 9, 2023

Choose a reason for hiding this comment

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

right, this will still take your files if you make a folder containing:

  • /Banc: what you've been passing to -d
  • /Ecosystem/FieldMapArea: what you've been passing to -b

I don't understand why people would want to extract into a structure that doesn't resemble the origin, since this loses context, but if this is still too much hassle then we can fix up the argument handling to take both ways. (done!)

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to require the folder structure to match the game's RomFS or extracted RomFS, so long as a full dump or compression is not required

@aquacluck
Copy link
Author

  • Explicitly prefer already-extracted files (was implicit)
  • Don't extract zsdics until it's known that we need them

@aquacluck aquacluck requested a review from leoetlino July 9, 2023 19:33
@aquacluck aquacluck force-pushed the misc-docker branch 2 times, most recently from 58e7109 to 31e3018 Compare July 9, 2023 19:48
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