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 for PCK patches to remove files #97356

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

reduz
Copy link
Member

@reduz reduz commented Sep 23, 2024

  • This was "kindof" existing in PCK but not properly done or supported. It seems this work was initiated over a decade ago and never completed.
  • If a file has offset zero in a PCK, it will be considered removed and it will be removed if previously added.

This adds the possibility to add proper directory level diffing to #97118.

@reduz reduz marked this pull request as ready for review September 23, 2024 10:42
@reduz reduz requested review from a team as code owners September 23, 2024 10:42
@AThousandShips AThousandShips added this to the 4.x milestone Sep 23, 2024
@mihe mihe force-pushed the pck-file-removal branch from 6accd43 to 7625f10 Compare November 8, 2024 18:32
@mihe mihe requested review from a team as code owners November 8, 2024 18:32
@mihe
Copy link
Contributor

mihe commented Nov 8, 2024

At the request of @reduz I've gone ahead and changed this PR to now integrate into the patch creation that was added in #97118. So now if you export a patch and files are found to have been removed since the last export (i.e. with all the patch packs loaded) it will automatically write these new file removal entries to the PCK.

I did have to change things around a bit from how they were originally in this PR though:

  1. A removal is no longer signified with a magic zero offset, as it turned out that a zero offset is in fact a valid offset (for the first file) since the offset is relative to the files section of the PCK and not the start of the PCK itself. Now instead we use a new PackFileFlags flag called PACK_FILE_REMOVAL.
  2. I removed the previous (incomplete) checking for offset in PackedData::try_open_path mentioned in the PR description. It serves no purpose as PackedData::files will never actually contain any removals with how this feature is done now.
  3. Because I'm forced to retrieve the list of previous files from PackedData, which doesn't store the actual paths but instead stores everything in a tree structure, I end up with a list of paths that are not only simplified (as in String::simplify_path) but that also have had any res:// scheme stripped from them. This leads to having to perform this sort of "normalization" elsewhere in order for certain comparisons and hashes to evaluate correctly, so you'll see .simplify_path().trim_prefix("res://") added in a number of places.

One benefit of that last point is that the paths stored in the PCK will from now on never include the res:// scheme, which should slim down PCKs a little bit. As mentioned above these schemes are being stripped when loaded into the PackedData tree structure anyway, so it looks to me like they are currently just bloating the PCK.

@akien-mga akien-mga changed the title Add ability for PCK to remove files Add ability for PCK patche to remove files Nov 11, 2024
@akien-mga akien-mga changed the title Add ability for PCK patche to remove files Add ability for PCK patches to remove files Nov 11, 2024
Copy link
Member

@akien-mga akien-mga 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.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Nov 11, 2024
Co-authored-by: Mikael Hermansson <mikael@hermansson.io>
@Repiteo Repiteo merged commit d76fbb7 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

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