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

Implement unload of PCK files #61286

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

Conversation

rsubtil
Copy link
Contributor

@rsubtil rsubtil commented May 22, 2022

Implements some ideas from godotengine/godot-proposals#2689.

This adds the functionality to unload any manually loaded pack from load_resource_pack, as well as querying if a specific pack is currently loaded.

The tricky part here is handling packs that replace files. The way I implemented some scenarios may not be what's intended, so I'll describe scenarios I found to ensure the behavior is intended. I also created a small project to help me test this, so if you want to play around and test these for yourself, here are the project files:
PCKTestingTool.zip

EDIT: Re-recorded video for better clarity

Peek.2023-07-01.12-13.mp4

On all these scenarios, there are 3 PCK files, all with different content on each scene, but with some duplicated names:

1.pck
└── a.tscn

2.pck
├── a.tscn
└── b.tscn

3.pck
└── b.tscn

Unloading pack which replaced some files from another one

  • Loading 2.pck:
a.tscn -> 2.pck
b.tscn -> 2.pck
  • Then loading 3.pck with replacing files enabled:
a.tscn -> 2.pck
b.tscn -> 3.pck
  • When unloading 3.pck, the files from 2.pck that were replaced are fetched and loaded again, essentially restoring the previous state:
a.tscn -> 2.pck
b.tscn -> 2.pck

Unloading pack which replaced files from an unloaded package

  • Loading 1.pck:
a.tscn -> 1.pck
  • Then loading 2.pck with replacing files enabled:
a.tscn -> 2.pck
b.tscn -> 2.pck
  • Then unloading 1.pck (nothing happens because all its files were replaced by 2.pck):
a.tscn -> 2.pck
b.tscn -> 2.pck
  • When unloading 2.pck, the files from 1.pck could have been replaced; however, because 1.pck has been unloaded in the meantime, none of it's files are reloaded:
<empty>

Unloading pack which didn't replace any existing files

  • Loading 1.pck
a.tscn -> 1.pck
  • Then loading 2.pck without replacing files:
a.tscn -> 1.pck
b.tscn -> 2.pck
  • When unloading 1.pck, the only files remaining are the ones 2.pck was able to load, leaving it in a partial state:
b.tscn -> 2.pck
  • Reloading 2.pck loads the missing files:
a.tscn -> 2.pck
b.tscn -> 2.pck

@Calinou
Copy link
Member

Calinou commented May 23, 2022

I think the testing project would be worth adding to the demo projects repository 🙂

@rsubtil
Copy link
Contributor Author

rsubtil commented May 23, 2022

I think the testing project would be worth adding to the demo projects repository 🙂

Will do! I'll clean up the code for it then 😉

@rsubtil
Copy link
Contributor Author

rsubtil commented Jul 7, 2022

Finally got a chance to work at this again 😌. I've added a demo at godotengine/godot-demo-projects#751

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Sep 8, 2022
rsubtil added a commit to retrohub-org/godot that referenced this pull request Jan 22, 2023
@Nordup
Copy link

Nordup commented Feb 12, 2023

Hi! What is the status on this pr? Curious when would it be merged because It's necessary for my project

@Calinou
Copy link
Member

Calinou commented Feb 12, 2023

Hi! What is the status on this pr? Curious when would it be merged because It's necessary for my project

4.0 is in feature freeze, so this can only be merged for 4.1 at the earliest.

In the meantime, you can pull the pull request's code into the master branch locally, compile a custom editor build and export templates for all platforms you plan to target.

@Anutrix
Copy link
Contributor

Anutrix commented Apr 16, 2023

Now that 4.0 is released, can we get this reviewed? There seems to people requesting this feature and there was effort to make a demo too.

@rsubtil
Copy link
Contributor Author

rsubtil commented Jun 24, 2023

This will need an update due to the new UID system (#50786). While it still works, since the UIDs among different sources for the same resource don't match (or replaced resources likely will get different UIDs), it generates a lot of warnings, and uses the file path as fallback.

I'm gonna need some time to find a solution to get these UIDs and account for them when loading PCK files. For now, I'll mark this as a draft since it's not ready to be merged.

@rsubtil rsubtil force-pushed the feature-unload_pck branch from a51032d to 13769ca Compare June 24, 2023 14:59
@rsubtil rsubtil marked this pull request as draft June 24, 2023 14:59
@rsubtil
Copy link
Contributor Author

rsubtil commented Jun 26, 2023

EDIT: moved to godotengine/godot-proposals#7181, see the next comment for reasoning.

@rsubtil rsubtil force-pushed the feature-unload_pck branch from 13769ca to ddfcb91 Compare July 1, 2023 11:16
@rsubtil
Copy link
Contributor Author

rsubtil commented Jul 1, 2023

After more investigation, it looks like the uid_cache.bin is already being force-exported on PCK files, and I got a version of solution 2 working, so I'll go with that approach:

String resource_cache_file = ResourceUID::get_cache_file();
if (FileAccess::exists(resource_cache_file)) {
files.push_back(resource_cache_file);
}

However, this issue occurs when loading PCK files, and doesn't concern anything from this PR, so I'll separate this to another issue/PR.

The unloading functionality is working as intended, so this PR is ready to merge.

@rsubtil rsubtil marked this pull request as ready for review July 1, 2023 11:22
@rsubtil
Copy link
Contributor Author

rsubtil commented Jul 22, 2023

This has a bug where if scripts from different packs share the same filename, their contents are still re-used even when packs are unloaded (likely gdscript_cache.cpp), I'll have a look at fixing this.

EDIT: Fixed

@rsubtil rsubtil force-pushed the feature-unload_pck branch from ddfcb91 to 87a38bd Compare July 22, 2023 20:53
@havi05
Copy link
Contributor

havi05 commented Feb 11, 2024

Is it realistic that this will be released with godot 4.3?

@Nordup
Copy link

Nordup commented Feb 11, 2024

Looking forward of it being merged too

@Calinou
Copy link
Member

Calinou commented Feb 12, 2024

Is it realistic that this will be released with godot 4.3?

We can't promise this will be merged for a specific release, as this still needs review from contributors knowledgeable with core parts of the engine. This is the area of engine which is critical to keep in a clean state since everything else relies on it, so there's naturally more resistance to change in that area too.

@AThousandShips AThousandShips changed the title Implemented unload of PCK files Implement unload of PCK files Feb 13, 2024
@SebMenozzi
Copy link

any update on this? I would love to see this merged one day 😊

@rsubtil rsubtil force-pushed the feature-unload_pck branch from 41d2f26 to 8ac6823 Compare November 9, 2024 12:53
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.