search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.7k stars 21.11k forks source link

Resources extended from GDExtension classes don't work right in 4.3 #93676

Closed aekobear closed 3 months ago

aekobear commented 4 months ago

Tested versions

System information

Godot v4.3.beta2 - Fedora Linux 40 (Workstation Edition) - Wayland - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2060 with Max-Q Design - AMD Ryzen 9 4900HS with Radeon Graphics (16 Threads)

Issue description

I created a GDExtension custom resource called MyResource. Then I created a GDScript resource called CustomResource which extends MyResource and has class_name CustomResource

In Godot 4.3 beta 2:

If I manually edit the TSCN to include the GDScript reference and then open it in Godot 4.3, everything works correctly until the next time the scene gets saved. So there seems to be some regression in the way resources are created and serialized

Steps to reproduce

# custom_resource.gd
extends MyResource
class_name CustomResource

function update():
  print("hello!")
# some_node.gd
@export var resources: Array[MyResource]

Minimal reproduction project (MRP)

IMPORTANT NOTE 1: deleting the .godot folder causes problems with the plugin on the initial load. You will need to reboot godot editor after loading the project for the first time before the plugin will start working

IMPORTANT NOTE 2: part of the bug is that resources aren't being serialized correctly on save, so the scene I upload here will be incorrect. To reproduce, you will need to:

1) Open the broken.tscn scene in the editor 2) Select Node2D in the tree. See how its resources variable has a single resource called MyResource? This is supposed to be a CustomResource but was serialized incorrectly. 3) Delete the MyResource and insert a new CustomResource instead. Hit play 4) Notice that instead of getting theprint()from CustomResource'supdate()function, you get an error thatupdate()doesn't exist onMyResource 5) Save and quit the editor 6) Reboot and notice that the resources array contains aMyResourceinstead ofCustomResource` again

The extension was written in rust using https://github.com/godot-rust/gdext

I included the compiled extension as well as source code for your reference

resource-bug.zip

akien-mga commented 4 months ago

CC @godotengine/gdextension @godotengine/gdscript

dsnopek commented 4 months ago

Thanks!

I had some trouble using the MRP locally, so I attempted to reproduce the bug in a new project using godot-cpp, and, unfortunately, I was unable to. I've attached my new project - perhaps, you can help me modify it in order to show the bug?

I tried the project in Godot 4.2.2, and the CustomResource resources stayed as CustomResources and had the expected methods when running the project. Then I tried in Godot 4.3-beta2, and even edited the "SomeNode" in the editor, and saved the project a bunch of times, but every time I ran it, it also had the expected methods and I didn't see the resources change type.

aekobear commented 4 months ago

I had some trouble using the MRP locally, so I attempted to reproduce the bug in a new project using godot-cpp, and, unfortunately, I was unable to. I've attached my new project - perhaps, you can help me modify it in order to show the bug?

Thanks for trying this out! I tested your cpp project and it does seem to be working as intended. Let me take this info back to the GDExt-rust people and see if there's a discrepancy on the rust side making it behave differently than godot-cpp

dsnopek commented 3 months ago

@Bromeon Can you check if this is a bug in the Rust bindings or something we broke in Godot 4.3 that affects the Rust bindings? As noted above, so far we haven't been able to reproduce it with godot-cpp. Thanks!

Bromeon commented 3 months ago

In Godot 4.3 beta 2:

  • when I run my game, CustomResource acts exactly like MyResource and does not have any of my custom behavior implemented in the GDScript.
  • when I save and close Godot, the .tscn file serializes the resource as MyResource with no reference to the CustomResource GDScript

In Godot 4.2 stable:

  • when I run my game, CustomResource behaves as expected
  • when I save and close Godot, the .tscn contains a script = ExtResource() pointing the resource to the appropriate GDScript

I downloaded your ZIP file, but couldn't quite follow these instructions. Even with 4.2.2, I get

SCRIPT ERROR: Invalid call. Nonexistent function 'update' in base 'MyResource'.
          at: _process (res://broken/broken.gd:8)

Are the initial scene files serialized correctly?

I would appreciate if you could reduce this example further, so that I can simply start it with:

$GODOT4_BIN --headless

and it would output "OK" or "FAIL" on the console. Then I wouldn't need to open the editor, save scenes, check diffs etc. Otherwise it's extremely time-consuming to bisect a larger range of versions.

Would that be possible? I'll only get around to look into it next week, so take your time.

dsnopek commented 3 months ago

With Bromeon's guidance, I changed the dependencies in the Cargo.toml so that I could build a version of the extension that would work in both Godot v4.2.2 and Godot v4.3-beta2, for the purpose of git bisecting to the commit that introduced the issue:

# Changed "api-custom" to "api-4-2"
godot = { git = "https://github.com/godot-rust/gdext", branch = "master", features = ["api-4-2"] }

However, when I tried in both Godot v4.2.2 and Godot v4.3-beta2, I was unable to reproduce the issue.

Perhaps the build was created on an older commit of the Rust bindings that had a bug?

@aekobear Can you try rebuilding the extension with the latest master of the Rust bindings and "api-4-2" and see if you're still experiencing the issue?

UPDATE: Here is a ZIP with the libextension.so that I built for Linux, in case anyone wants to try dropping that into the MRP.

akien-mga commented 3 months ago

So I've been doing some more testing and it's pretty confusing, so the steps to reproduce are important to get right. Initially I tested with @dsnopek's api-4.2 build and it worked fine, and then loading it up in custom builds made with api-custom and 4.3-beta2 and latest master also seemed to work fine.

But I suspect the broken.tscn scene had been fixed by loading it with the 4.2 lib, and stayed fixed afterwards. But changing that scene with a 4.3 lib does recreate the issue.

Basically, with the 4.2 lib, you get:

[gd_scene load_steps=4 format=3 uid="uid://t1i4yl2r35cw"]

[ext_resource type="Script" path="res://broken/broken.gd" id="1_3406y"]
[ext_resource type="Script" path="res://broken/custom_resource.gd" id="2_jfebs"]

[sub_resource type="MyResource" id="MyResource_6iub0"]
script = ExtResource("2_jfebs")
speed = 2.0

[node name="Node2D" type="Node2D"]
script = ExtResource("1_3406y")
resources = Array[MyResource]([SubResource("MyResource_6iub0")]

While with the 4.3 lib, you get:

[gd_scene load_steps=3 format=3 uid="uid://t1i4yl2r35cw"]

[ext_resource type="Script" path="res://broken/broken.gd" id="1_3406y"]

[sub_resource type="MyResource" id="MyResource_dp1iy"]
speed = 2.0

[node name="Node2D" type="Node2D"]
script = ExtResource("1_3406y")
resources = Array[MyResource]([SubResource("MyResource_dp1iy")])

which fails, as the subresource is of type MyResource when it should be CustomResource (via ExtResource).

Steps to reproduce

For all tests, use a Godot editor build from latest master, for the avoidance of doubt on whether e.g. #93384 would be involved, as I originally assumed (it's not). I'm using e6448ca0aa050b71cb40d9658b7233b8ec7f7382.

Reproduce with the MRP

Fix the bug with the 4.2 lib

Here's my version of the "fixed" MRP: resource-bug-api-4.2.zip

Reintroduce the bug with the 4.3 lib

Follow the steps precisely here as data gets lost progressively.

[ext_resource type="Script" path="res://broken/broken.gd" id="1_3406y"]

[sub_resource type="MyResource" id="MyResource_r8cxc"] speed = 2.0

[node name="Node2D" type="Node2D"] script = ExtResource("1_3406y") resources = ArrayMyResource


- Try to run it, the "update" function error is back.
- Reopen Godot, see that the scene is back at:
![image](https://github.com/godotengine/godot/assets/4701338/5fb0ae4f-7ee5-49fc-a756-b7c9ab99e495)
and that saving it loses the `speed = 2.0` part in the `.tscn`, since it's not a property of `MyResource`.

I could reproduce this issue with both the original lib in the OP's MRP built against 4.3-beta2, and with my custom build made against latest `master` (e6448ca0aa050b71cb40d9658b7233b8ec7f7382).

---

So that confirms a saving and loading issue with 4.3 for custom resources extending godot-rust GDExtension classes.
Hilderin commented 3 months ago

I worked on this issue and compared Rust and C++ GDExtension.

I confirm that the Rust version does not work but the C++ version is working fine on master branch.

The difference is in the set and get method of the extension. In Rust extension _extension->get and _extension->set called in Object::get/set always returns true when passing the script property. In C++, it returns false probably because it's not an existing property of the resource. Rust probably do not check if the property exists.

If _extension->get returns true, then the method set_script is never called in Object::set. If _extension->set returns true, then the method get_script is never called in Object::get.

I think the problem is a side effect from #82554. With the new PlaceholderExtensionInstance, the set and get methods always returns true even if the property does not exists.

I made a branch with a little tweak to test the theory and with this modification Rust GDExtension Resource are working fine: https://github.com/godotengine/godot/compare/master...Hilderin:godot:fix-resources-extended-from-gdextension-classes-

I guess the problem could be fixed in Rust side but I think the behavior of PlaceholderExtensionInstance is not perfect since it differs from 4.2 and what is expected. Probably a better fix could be that PlaceholderExtensionInstance::get/set returns true only if the property really exists?

These are the MRP projects that I used to test: I only built for Windows and with the master branch. Rust: test-godot-rust-extension-resource-bug - MRP.zip

C++: test-godot-c++-extension-minimal_repro - MRP.zip

In both project, open de Broken.tscn

To reproduce:

dsnopek commented 3 months ago

Ah, if placeholders are involved, then I guess Rust is making the class as a runtime class? I didn't pick up on that from the source code.

Thanks for the further investigation!

Probably a better fix could be that PlaceholderExtensionInstance::get/set returns true only if the property really exists?

The tricky thing is that we don't actually know if a property exists or not. When using runtime classes, the real class is not actually created in the editor, only a placeholder. So, we'll know about "static properties", but the class could use get(), set() and get_property_list() to make "dynamic properties", and we don't want to make it impossible to set a dynamic property in the editor that would exist when the game actually runs. But I think we could probably make some assumptions...

The script API has its own placeholders (via PlaceHolderScriptInstance) for implementing non-@tool classes, and we could try implementing similar logic as PlaceHolderScriptInstance::set() and PlaceHolderScriptInstance::get().

Later I'll try the MRPs and see if I can come up with logic that would work in this case, and with my other tests with runtime classes.

dsnopek commented 3 months ago

I just posted PR https://github.com/godotengine/godot/pull/94089 which should fix this.

In the end, I don't think we actually need to worry about "dynamic properties": since only the placeholder exists in the editor, there wouldn't really be any way to set them anyway. I also noticed a couple other issues/limitations with runtime classes when digging into this that should be addressed by that PR.

I tested with my own godot-cpp test project - the most recent C++ MRP from above didn't actually reproduce the problem for me, maybe because it didn't register its custom class as a runtime class? Anyway, it was very easy to reproduce once I knew this was about runtime classes. I've attached my test project.

Please let me know if Godot with this PR fixes the issue with Rust in your testing!

aekobear commented 3 months ago

I just posted PR #94089 which should fix this. ... Please let me know if Godot with this PR fixes the issue with Rust in your testing!

@dsnopek I compiled/ran my Rust MRP against your PR and it's working perfectly for me now :tada:

Many thanks to you and everyone in this thread for figuring this out!