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

Type casting of certain resources fails unexpectedly #73615

Open
Tracked by #80877
Pennycook opened this issue Feb 20, 2023 · 5 comments
Open
Tracked by #80877

Type casting of certain resources fails unexpectedly #73615

Pennycook opened this issue Feb 20, 2023 · 5 comments

Comments

@Pennycook
Copy link
Contributor

Pennycook commented Feb 20, 2023

Godot version

v4.0.rc2.official [d2699dc], 4.0.rc3.official [7e79aea], v4.0.rc4.official [e0de357].

System information

Steam OS Holo, Steam Deck

Issue description

Since moving my project over to 4.0, I've encountered several unexpected failures when trying to cast Resource objects to custom types. I don't really understand what is going on (and the bug may be in my code, rather than Godot)... But what I've observed is that these casts can fail because of the presence of seemingly unrelated code that never even runs.

Steps to reproduce

I've done my best to create a minimal reproducer (see attached).

The two most important files are below. Running the project as provided in the attached zip causes the assert in the Inventory initializer to fail; the Resource is apparently not an Item. Commenting out the declaration of item inside foo allows the assert to start passing.

Test.gd:

extends Node2D

@onready var _inventory = preload("res://Inventory.gd").new()

# NB: foo doesn't even need to be called for this to trigger
func foo(variant):
	# Comment the line below to make the assertion in Inventory.gd pass
	var item : Item = variant
	pass

Inventory.gd:

class_name Inventory
extends Resource

var contents : Dictionary = {
	preload("res://TestItem.tres") as Item: 0,
}

func _init():
	print_debug("Initializing Inventory")
	for k in contents.keys():
		assert(k is Item)

Minimal reproduction project

ResourceAsKey.zip

@Chaosus Chaosus added this to the 4.0 milestone Feb 20, 2023
@Pennycook
Copy link
Contributor Author

This is still an issue with v4.0.rc3.official [7e79aea].

It also affects the new is_instance_of function; the assert still fails if rewritten as:

assert(is_instance_of(k, Item))

I've also uncovered a few additional workarounds, although I'm not sure why they work:

  • Using load instead of preload (for either Inventory.gd or TestItem.tres)
  • Adding an unused preload of Item.gd prior to the preload of Inventory.gd, as below:
    const Foo = preload("res://Item.gd")

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 22, 2023
@Pennycook
Copy link
Contributor Author

@akien-mga - Out of curiosity, why did this get bumped from 4.0 to 4.1?

I've just tested this with Godot 3.5.1 and it worked, so this is a regression that could break existing projects.

@akien-mga
Copy link
Member

We're at RC4, so the 4.0 release is imminent. There's only so many issues we can expect to solve (without introducing regressions) in a few days.

This one has documented workarounds, so it doesn't appear release blocking.

@vnen
Copy link
Member

vnen commented Feb 24, 2023

At a quick glance this seems related to cyclic dependencies. Using class names in scripts are generally fine, but when you start preloading things you introduce hard dependencies that can't really be worked around. Preloading GDScript files mostly work like class names, it gets difficult when you get to preloading resources that include scripts.

I don't really know how that works in 3.x TBH, since it's more stingy on cyclic dependencies.

@Pennycook
Copy link
Contributor Author

We're at RC4, so the 4.0 release is imminent. There's only so many issues we can expect to solve (without introducing regressions) in a few days.

Makes sense. Thanks for the explanation.

At a quick glance this seems related to cyclic dependencies. Using class names in scripts are generally fine, but when you start preloading things you introduce hard dependencies that can't really be worked around

Thanks for looking at this, @vnen. I agree that this smells like (and behaves like) a cyclic dependency, but I don't think there's actually a cycle anywhere.

For the case that fails, the dependencies look like this:

flowchart TB
    Test.gd -->|preloads| Inventory.gd
    Inventory.gd -->|preloads| TestItem.tres
    TestItem.tres -->|is instance of| Item.gd
    Test.gd -->|declares variable of type| Item.gd
    Inventory.gd -->|declares variable of type| Item.gd
Loading

For the case that works (with the workaround), the dependencies look like this:

flowchart TB
    Test.gd -->|preloads| Inventory.gd
    Test.gd -->|preloads| Item.gd
    Inventory.gd -->|preloads| TestItem.tres
    TestItem.tres -->|is instance of| Item.gd
    Test.gd -->|declares variable of type| Item.gd
    Inventory.gd -->|declares variable of type| Item.gd
Loading

I guess there might still be some order-of-include problem somewhere, but I don't know enough about how GDScript is implemented to understand why this might happen.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 22, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.x Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants