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 ability to omit redundant data for additional packs #89928

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Mar 27, 2024

(Note that this PR has gone through a couple of iterations now, in case some of the conversation here doesn't quite line up.)

This reintroduces the trimming of packs introduced by #82084, which was later reverted in #90476.

This is now instead opt-in and toggled through a checkbox in the "Export PCK/ZIP" file dialog, titled "Export As Additional Pack", similar to the existing "Export With Debug" checkbox. This new checkbox is disabled/unchecked by default.

image

This PR also exposes a new command-line flag called --export-additional-pack <preset> <path>, which functions in much the same way as --export-pack <preset> <path> except it enables the same trimming as this new file dialog checkbox.

Documentation changes can be found in godotengine/godot-docs#9147.

TODO

  • Add a new command-line flag to allow switching this behavior from CI workflows and whatnot.
  • Document all of this.

@ogapo
Copy link
Contributor

ogapo commented Mar 27, 2024

Seems like a good quick fix. I do think at a minimum something like support for --main-pck=true/false (as mentioned in your to-do) or being able to declare it in the export config would be necessary. Otherwise we risk losing command line parity with the UI (given exporting is often automated this strikes me as crucial).

@mihe

This comment has been minimized.

@mihe mihe force-pushed the main-pack-checkbox branch from e076df4 to 75517e1 Compare March 27, 2024 16:43
@mihe mihe changed the title Add main pack checkbox to PCK/ZIP export dialog Change pack trimming to be opt-in Mar 27, 2024
@mihe

This comment has been minimized.

@mihe mihe force-pushed the main-pack-checkbox branch from 75517e1 to 9439fa0 Compare March 27, 2024 17:32
@mihe

This comment has been minimized.

editor/export/project_export.cpp Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
@mihe mihe marked this pull request as draft April 10, 2024 10:51
@mihe mihe force-pushed the main-pack-checkbox branch from 9439fa0 to 7e7447b Compare April 10, 2024 16:24
@mihe mihe changed the title Change pack trimming to be opt-in Add ability to omit redundant data for additional packs Apr 10, 2024
@mihe mihe force-pushed the main-pack-checkbox branch from 7e7447b to 26252eb Compare April 10, 2024 17:31
@mihe
Copy link
Contributor Author

mihe commented Apr 11, 2024

This now apparently ended up having quite a lot of overlap with #64712, which I was just made aware of, which to me looks like a much better approach to achieve this kind of "pack trimming".

@ogapo
Copy link
Contributor

ogapo commented Apr 11, 2024

I like the idea of being able to create patches easily because it's a common pattern but patching doesn't really cover the use case of DLC where you're not necessarily diffing anything you're just trying to split the contents into multiple pck files so they are independent. In theory those could also want to be diffs so it feels like it might be orthogonal.

That said I think holistically it might make more sense to enumerate all the PCKs as part of the export config (so they could all be generated at once) but that'd be a bigger rethink of the workflow here.

@mihe
Copy link
Contributor Author

mihe commented Apr 11, 2024

I like the idea of being able to create patches easily because it's a common pattern but patching doesn't really cover the use case of DLC where you're not necessarily diffing anything you're just trying to split the contents into multiple pck files so they are independent. In theory those could also want to be diffs so it feels like it might be orthogonal.

Hm, yeah, fair point. I do feel like the UX of having both of these options sitting side-by-side is bound to be confusing though.

Something as simple as just renaming the option/argument in #64712 to be "Export as Additional Pack" and --export-additional-pack would presumably allow for this PR to be integrated into that one quite cleanly, by always omitting files like project.binary, icon.svg, etc. whenever that option is checked.

That way you could just not have any packs listed under "Patches" if you don't want to do any diffing, which seems to work just fine with that PR, as far as I can tell.

@ogapo
Copy link
Contributor

ogapo commented Apr 11, 2024

True that would be pretty clean imho. Diffing against nothing should be allowed and putting anything in that list should trigger a diff (they don't need a checkbox really). Then the checkbox remains the main vs aux pack switch you added.

@akien-mga akien-mga modified the milestones: 4.3, 4.4 May 29, 2024
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.

6 participants