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

Fix patch PCKs incorrectly marking imported resources for removal #102099

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jan 27, 2025

Fixes #102098.

This fixes an issue when exporting patch PCKs, where imported assets would incorrectly end up being marked for removal.

The fundamental problem was that the code (introduced by me in #97356) that compared against the currently loaded PackedData in EditorExportPlatform::export_project_files had an incomplete view of the files that had actually been saved to the newly written PCK, which resulted in them being interpreted as missing and thus written out as removals to the patch PCK. On top of that, this code didn't actually look at the "final" exported paths but instead relied on various intermediate lists, which was likely quite brittle to begin with.

This pull request resolves this by adding a EditorExportSaveProxy class that's meant to be used (purely internally) for all file saves happening in EditorExportPlatform::export_project_files, effectively "detouring" the EditorExportSaveFunction function pointer that's passed into EditorExportPlatform::export_project_files. This class then tracks every file saved, in the appropriate "normalized" format of *.simplify_path().trim_prefix("res://"), which is then referenced at the end when checking to see which files are missing and should be marked for removal in the patch PCK.


This also happens to resolve a performance regression that was introduced (once again, by me) as part of #97356, when exporting patches, as we were doing a linear search of potentially thousands of loaded files across multiple lists, some of which could be in the thousands themselves. This was also made worse by the fact that we normalized every path (using the above mentioned format) as we compared against it.

Now with every written out path stored in EditorExportSaveProxy::saved_paths, in the format we'd expect it to be when loaded, we can omit any sort of normalization and just do a single quick HashSet lookup for every loaded file, greatly alleviating this performance regression.


I should probably also mention that this EditorExportSaveProxy could technically be replaced entirely by a reference-capturing lambda that wraps p_save_func, but given the aversion to lambdas I opted to go this route instead. I'd be curious to hear any other suggestions for a cleaner solution.

@mihe mihe added this to the 4.4 milestone Jan 27, 2025
@akien-mga akien-mga requested a review from bruvzg January 27, 2025 22:31
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected, code looks good. Using EditorExportSaveProxy seems fine to me.

@Repiteo Repiteo merged commit d421ccc into godotengine:master Jan 28, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 28, 2025

Thanks!

@mihe mihe deleted the pck-patch-bug branch January 28, 2025 15:12
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.

Exported patch PCKs incorrectly mark imported resources for removal
3 participants