`class_name` classes become invalid on PCK scenes when unloading PCK files

rsubtil commented 11 months ago

Godot version

latest, with https://github.com/godotengine/godot/pull/61286 applied

System information

Arch Linux

Issue description

[!Note] This issue is only relevant with the functionality of https://github.com/godotengine/godot/pull/61286 applied

When implementing the functionality to unload PCK files (which involves clearing GDScript cache of the loaded pack to clear scripts; not sure if that's the root cause), reloading such files results in an error where a custom class_name class that's defined on the main project, works on the main project, but when running code on the loaded pack, Godot complains the class is not from the correct type:

This only occurs when the pack in unloaded; when the pack is loaded for the first time, or is reloaded without unloading it first, this issue doesn't occur.

Steps to reproduce

Have the main project and sub-project define some custom resource using class_name:
```
extends Resource
class_name MyResource
```

var text := "default"

- In the sub-project, define some scene to be loaded, and a method receiving the custom object:
![image](https://github.com/godotengine/godot/assets/6501975/2d6584ad-ebaa-48ff-923b-721975d8fe4e)
```gdscript
extends Control

func got_info(info: MyResource):
    $Label.text = info.text

Export the sub-project as a PCK file.

In the main project, load the PCK file, instance the scene, then call that method:

func load_pck():
print("Result: ", ProjectSettings.load_resource_pack(pck_file), false)

var child = load("res://scene.tscn").instantiate()
$SubViewportContainer/SubViewport.add_child(child)

var my_res := MyResource.new()
my_res.text = "test"
child.got_info(my_res)

So far so good; there's no issues with reloading this pack file

Now add functionality on the main project to unload the PCK file before reloading it.
```
func _on_button_pressed():
await unload_pck()
load_pck()
```

func unload_pck(): for child in $SubViewportContainer/SubViewport.get_children(): $SubViewportContainer/SubViewport.remove_child(child) child.queue_free() while is_instance_valid(child): await get_tree().process_frame

print("Result: ", ProjectSettings.unload_resource_pack(pck_file))

- Now, when reloading the same scene, the issue will occur when `got_info` is called on the instanced child object.

----
Some other scenarios I've tested:
- This issue still occurs if the pack is allowed to replace files.
- This issue does not occur if in the `got_info` method, the argument type is not specified:
```gdscript
extends Control

func got_info(info):
    $Label.text = info.text

On #61286, commenting the part that clears the pack's script cache also stops this issue from occurring. However, it's not a solution since scripts need to be cleared when a pack is unloaded. https://github.com/godotengine/godot/pull/61286/files#diff-f45df99ed947ca0517c7b926532a8c1a3d27aeda5797c510da673e10462d75cfR191-R195
When backporting this functionality to 3.x, there are no issues whatsoever. Might be because 3.x didn't have this GDScript caching functionality.

Minimal reproduction project

IssueRepro.zip

[!Note] Needs https://github.com/godotengine/godot/pull/61286 applied in order to get the unload pack functionality

This contains two projects: the main project on root, and sub-project on inner folder. The sub-project is already exported at inner/inner.pck. Run the main project, and click on the Load button to reload the pack and trigger the issue.

rsubtil commented 11 months ago

Forgot to mention, this issue looks very similar to https://github.com/godotengine/godot/issues/62982

willnationsdev commented 11 months ago

In some sense, this is expected behavior, isn't it? There's a script that has a static type hint for a pck-based script; when that pck is unloaded, the static type hint now fails to be valid because the type doesn't exist anymore (as far as the language is concerned). Is that right? I can see that you are unloading and then immediately reloading the pck, but because these take place asynchronously, I imagine the process does not fully complete before GDScript's background logic re-triggers the scanning/analyzing/caching of GDScript resources. Would you not need to have a single C++ method exposed (like a transaction) in order for GDScript to perform operations that synchronously complete the entire unloading/loading process? Perhaps you'd even need the GDScript language to have a flag that can be set to postpone scanning until such pck operations are no longer in flux.

rsubtil commented 11 months ago

In some sense, this is expected behavior, isn't it? There's a script that has a static type hint for a pck-based script; when that pck is unloaded, the static type hint now fails to be valid because the type doesn't exist anymore (as far as the language is concerned). Is that right?

You're right! I though this was not possible because the pack had replace_files disabled, and in theory that shouldn't happen. But from what I tested now, that's not the case; Godot is loading those files and overriding them despite replace_files being disabled. So I did now a deeper investigation to figure out what's going on.

When running in the editor, Godot was able to load all files from the pack, because there is no main pack file. This logic only replaces files loaded by other packs, but at the editor level Godot simply mounts the project dir. So, when loading the inner PCK it would overwrite that res://my_resource.gd, and when unloaded, remove it and invalidate the class definition. This does not happen on an exported version though; because Godot first loads the main resources from the main PCK now, those files are registered as being owned by a pack, and thus the inner PCK now can only load files that don't replace any other, no longer causing this issue!

So I guess the issue is not with unloading PCKs at all, but rather that when running on the editor, PCKs are effectively being able to replace files because they "don't exist", as in "no other pack loaded them before".

This was a pretty big blocker for my project, so thank you so much for the help! I'll investigate this a bit further before closing the issue to ensure all is indeed working without issues.

rsubtil commented 11 months ago

Alright, so I've tried to restore this functionality to my project, but sadly the issue persists :cry:

I've now noticed that the error I mentioned is not the correct one. I don't know if they are the same in practice, but my error is actually ERROR: Error calling from signal 'info' to callable: 'Control(Label.gd)::got_info': Cannot convert argument 1 from Object to Object.. This error pops up when this custom resource is propagated through a singleton signal to code on inner PCK, instead of calling a method directly, in an extremely similar way of https://github.com/godotengine/godot/issues/62982.

Interestingly though, this issue is fixed on this reproduction test (even when changing it to a singleton signal), so I'll need to figure out another way of replicating this problem, and update the issue accordingly.

godotengine / godot