Merge uid_cache.bin and global_script_class_cache.cfg after mounting PCKs

[ogapo]

Fix for uid_cache.bin and global_script_class_cache.cfg not taking into account DLC mounting

The global script and UID caches were not getting updated after PCK files mount. This resulted in scripts using global class_name not being able to resolve properly as well as UID->resource path mappings not getting stored. I added methods to ProjectSettings and ResourceUID that allow merging in newly updated copies of these global caches. These functions intentionally merge rather than replace to account for the case where multiple DLC/MODs may have independent information, but not contain eachother's information.

This fixes #82061.

Removed some unnecessary files from non-main PCKs

In addition, non-main PCK files presently include a lot of cruft that can't be used after startup and just bloats DLC. It's desirable for downloaded content to be as small as possible. This change avoids bloating non-main pack files with new versions of resources that are all read on startup and never used again. These have no effect if replaced after startup:

• project.godot/project.binary file
• extension_list.cfg
• app icon and boot_splash
• .ico and .icns files (these can still be opted in for DLC by listing them explicitly in the include filter)

Added a missing error feedback to PCK export

Finally, while I was in here I noticed the --export-pack command doesn't provide any feedback if you accidentally use a non-supported extension (other than .pck or .zip) so I error messaging and set the status code properly.

[ghmart]

Not a reviewer, but I tested this fork with my project and can confirm that this fix makes all UIDs valid.

No more need to use workaround like what was mentioned in #82061.

Thank you for this fix! Hope to see it merged with master soon.

[rsubtil]

This should close #61556 and superseed my fix for UIDs at #79076 (also closing godotengine/godot-proposals#7181)

It seems though this will only work with packs loaded with replace_files enabled, and so I think those two files need to be fetched regardless of that setting, in order to work on all scenarios.

[ogapo]

@rsubtil Good observation about when replace_files=false.

I started to try and handle this as feedback, but unfortunately I am not sure my approach can work at all in the case where we're not replacing files. If we don't replace, then some files may not get their new versions mounted which means some of the UIDs in the UID cache, and some of the classes in the class cache may not be correct.

Without a more extensive change (possibly to those files themselves) I'm not sure the behavior would be better than leaving it as is. Seems like having no data in the cache would be better than wrong data. I think it might be best to leave it as a known issue and try to address this case in a follow-up PR.

[rsubtil]

I started to try and handle this as feedback, but unfortunately I am not sure my approach can work at all in the case where we're not replacing files. If we don't replace, then some files may not get their new versions mounted which means some of the UIDs in the UID cache, and some of the classes in the class cache may not be correct.

Indeed, it's actually the reason I opened a proposal rather than an issue to get some opinions on how to tackle this. My solution was to merge both UID caches regardless of files being replaced. From what I can tell (and tested so far), even if an UID of a new resource gets registered, and that resource wasn't allowed to replace existing ones, it's extremely unlikely that the original project will use that UID that's now pointing to a non-existing resource. There is always that possibility, but likewise the current UID system as it stands can also result in UID collisions, so knowing that I went ahead with this solution.

Now, for global classes, I agree I'm not sure this would work, considering they store more information that is likely to break stuff if it were replaced (notably path, which could begin pointing to a non-loaded resource). In that scenario I think existing classes shouldn't be replaced if replace_files=false, but it needs to be tested.

So I agree to leave this as is, and investigate further how to tackle non-replacing packs. I'm actually depending on that behavior, so I'll be able to test this more extensively in the future.

[ogapo]

Squashed commits per https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html

[AThousandShips]

You haven't squashed your commits

[AThousandShips]

You've included several of the commit messages after the body of your message, please remove them to restrict it to:

Distinguish between main pack and DLC packs.
It's desirable to downloaded content to be as small as possible. This change avoids bloating non-main pack files with new versions of resources that are all read on startup and never used again. They have no effect if loaded after startup.
- project.godot/project.binary file
- extension_list.cfg
- app icon and boot_splash
- .ico and .icns files (these can still be opted in for DLC by listing them explicitly in the include filter)

[ogapo]

@AThousandShips My bad. Had some git aggro haha. I believe it's in the correct state now.

[AThousandShips]

It still contains what looks like random comments on bug fixes you did:

Refresh the global class list after loading a PCK

merge in the new uid cache when a PCK file loads

call add_global_class for all cases because base/language/path may change.
modified add_global_class to only set inheriters_cache_dirty if something actually updates since StringName comparisons are cheap

The commit message should be brief and specific :)

[ogapo]

Ok, I've cleaned that up a bit. Let me know if you'd like further changes. That first quote doesn't fully summarize the change. Main fix is the merging of the global class and UID cache, the restriction of files is complimentary.

[AThousandShips]

LGTM

A commit message doesn't have to explain the PR generally, just give a basic idea of what's done, the code and the PR should convey the real details most of the time

[akien-mga]

Should this be in each pck too, or only the main pack?

If only the main pack, this is starting to look like it should just be two separate functions as there's little overlap between the two scenarios.

[ogapo]

To be honest, I wasn't sure how this worked. Conceptually, if this is a translation DB, then it probably needs to be in all PCKs since they could potentially introduce new strings. That said, it probably would need a similar merge process as the UID cache and global script class cache to work properly. I'm not really comfortable enough with the localization system to attempt that fix.

In lieu of that, I can either pack it in the main pck only (trimming something that's possibly unused) or keep it in all pcks (preserving current behavior). Thoughts? Happy to split this function in two if the call is to main-pck it.

[akien-mga]

I believe it's a dictionary that's not specific to the game translations / pck strings, but @bruvzg should be able to confirm.

[ogapo]

Applied the suggested changes (thanks @mihe).

[mihe]

LGTM. You'll need to squash it all into a single commit, in order for it to be merged.

[ogapo]

Squashed.

@mihe is there anything else you need from me? Github still says merging is blocked (pending an approving review) but it looks like you approved it. Just trying to wrap my head around the process here.

[AThousandShips]

No maintainers have approved this yet :)

[ogapo]

No maintainers have approved this yet :)

Oh derp. Sorry I just got a notification and assumed heh. waits patiently

[akien-mga]

Code changes look good to me in the latest version, and I trust @mihe's and @dreed-sd's testing.

Could be good to have maybe @bruvzg check it too, but overall I don't see blockers to merge soon.

[akien-mga]

Thanks!

[aaronfranke]

This PR caused a serious regression where PCK files do not work anymore: #89825

[mihe]

A fix is lined up in #89928 already, but I guess it needs a review and/or test from someone else.

If the UX changes in that one are considered too much at this stage we can simply revert the trimming part of this PR, make it a part of #89928 instead and save that one for 4.4 I guess.

Thoughts?

[akien-mga]

If the UX changes in that one are considered too much at this stage we can simply revert the trimming part of this PR, make it a part of #89928 instead and save that one for 4.4 I guess.

I think we can still agree on changes for 4.3, but for the next dev snapshot that approach sounds good to me.

[mihe]

The revert can be found in #90476.