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

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

Closed
aekobear opened this issue Jun 27, 2024 · 11 comments · Fixed by #94089
Closed

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

aekobear opened this issue Jun 27, 2024 · 11 comments · Fixed by #94089

Comments

@aekobear
Copy link
Contributor

Tested versions

  • Reproducible in 4.3 beta 2
  • Not reproducible in 4.2 stable

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:

  • 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

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

  • I created a custom GDExtension class (in rust) which extends Resource. Let's call it MyResource

  • Then, in GDScript I created a new Resource like this:

# custom_resource.gd
extends MyResource
class_name CustomResource

function update():
  print("hello!")
  • Then I created a separate GDScript which references an array of the resources
# some_node.gd
@export var resources: Array[MyResource]
  • Finally, using the editor, I click the button to add a new CustomResource to the array

  • I am able to set variables on the CustomResource as expected, but when I run the game, it acts as if the resource were a MyResource. And when I close and reboot Godot, the editor now displays it as a MyResource instead of a CustomResource

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 the print() from CustomResource's update() function, you get an error that update() doesn't exist on MyResource
  5. Save and quit the editor
  6. Reboot and notice that the resources array contains a MyResource instead of CustomResource 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
Copy link
Member

CC @godotengine/gdextension @godotengine/gdscript

@dsnopek
Copy link
Contributor

dsnopek commented Jul 1, 2024

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
Copy link
Contributor Author

aekobear commented Jul 2, 2024

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
Copy link
Contributor

dsnopek commented Jul 2, 2024

@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
Copy link
Contributor

Bromeon commented Jul 3, 2024

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
Copy link
Contributor

dsnopek commented Jul 3, 2024

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
Copy link
Member

akien-mga commented Jul 4, 2024

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 e6448ca.

Reproduce with the MRP

  • Unzip the MRP.
  • Open in Godot, see GDScript bug: Native class "MyResource" not found. errors ("normal" errors on first load of a gdextension sadly, at least it's a separate bug), restart Godot.
  • Try to run it, see the update function error.
  • The broken.tscn looks like this:
    image
  • Fix it to this. Using 2 as speed value to know whether it's properly serialized and loaded, as opposed to just a default instance.
    image
  • Save, and see that the broken.tscn text file still has no reference to res://broken/custom_resource.gd, i.e. the scene isn't properly saved.
  • Try to play the project, see the bug still happens.
  • Reopen Godot, see that the bug still happens.

Fix the bug with the 4.2 lib

  • Unzip the MRP in a new location.
  • Replace the libextension.so lib by the one @dsnopek built against api-4.2.
  • Open in Godot, see GDScript bug: Native class "MyResource" not found. errors ("normal" errors on first load of a gdextension sadly, at least it's a separate bug), restart Godot.
  • Try to run it, see the update function error.
  • Fix the broken.tscn like in the previous test.
  • Save, and see that the broken.tscn text file HAS the proper reference to res://broken/custom_resource.gd.
  • Try to play the project, there's no bug, it prints speed is: 2.
  • Reopen Godot, see that the broken.tscn scene is still correct and the project plays fine.
  • Make a copy of that MRP for future reference if needed, as a good state to start from when testing other lib builds.

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.

  • Reuse the "fixed" MRP using the api-4.2 lib.
  • Replace the libextension.so lib by the one from the original MRP, built against the 4.3-beta2 API.
  • Run the project from the commandline without opening it in the editor --> works OK, confirm that the broken.tscn text file looks good.
  • Open the project in the editor, and see that the broken.tscn scene lost some data (see screenshot). Do not save at this stage. Check the broken.tscn text file, which still looks correct.
    image
  • Run the project from the editor, it triggers a save and loses some data, namely the speed = 2.0 part. But it still has the ExtResource reference, so the code works fine and prints speed is: 1 (partial breakage).
  • Without modifying the scene, save it, play, reopen Godot, etc. --> no change, still prints speed is: 2.
  • Edit the scene again to have a CustomResource with speed 2:
    image
  • Save, and see that it's saved wrongly as a MyResource with speed = 2.0:
[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_r8cxc"]
speed = 2.0

[node name="Node2D" type="Node2D"]
script = ExtResource("1_3406y")
resources = Array[MyResource]([SubResource("MyResource_r8cxc")])
  • Try to run it, the "update" function error is back.
  • Reopen Godot, see that the scene is back at:
    image
    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 (e6448ca).


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

@Hilderin
Copy link
Contributor

Hilderin commented Jul 6, 2024

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: 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:

  • Open Broken.tscn
  • Remove MyResource resources in property of root node
  • Add a new local CustomResource in the property
  • Save the scene
  • In Rust version, the Broken.tscn will be broken, the script line will not be present in sub_resource
  • If you close and reopen the Editor, the CustomResource are now MyResource(rust)/ExempleResource(c++) (even if the scene file is correct).

@dsnopek
Copy link
Contributor

dsnopek commented Jul 6, 2024

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
Copy link
Contributor

dsnopek commented Jul 8, 2024

I just posted PR #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
Copy link
Contributor Author

aekobear commented Jul 8, 2024

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 🎉

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Very Bad
Development

Successfully merging a pull request may close this issue.

5 participants