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

Some usages of class_name can produce cyclic errors unexpectedly #21461

Closed
ghost opened this issue Aug 26, 2018 · 81 comments · Fixed by #67714
Closed

Some usages of class_name can produce cyclic errors unexpectedly #21461

ghost opened this issue Aug 26, 2018 · 81 comments · Fixed by #67714

Comments

@ghost
Copy link

ghost commented Aug 26, 2018


Bugsquad edit: This issue has been confirmed several times already. No need to confirm it further.


Windows 10 64-bit - Godot 3.1 alpha

Edit - Refined issue to focus on cyclic errors.

Certain uses of class_name can produce cyclic errors in situations where there are no cyclic references, and in fact sometimes no other references outside of the single one given.

I've noticed this most frequently in tool script when using is to make a class comparison, but it seems to happen at times in normal script situations.

Recreating it in a sample project has eluded me.

godot master_2018-08-27_00-47-02

@PetePete1984
Copy link
Contributor

It'll work if you give the base script a class_name and use that in the extends line.
I'm also getting the error when using file names at first (https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_parser.cpp#L4947), but it also disappears and doesn't come back after it worked once (as in, after I use the mentioned workaround)

@ghost
Copy link
Author

ghost commented Aug 26, 2018

@PetePete1984 Cool, thanks Pete. This is how it is intended to be used then?

While double checking and testing this, I was looking at the get_class() method, and wondering if it should return Node2D? The is CustomClass part is returning true, but is_class("CustomClass") on the other hand is false.

@akien-mga akien-mga added this to the 3.1 milestone Aug 27, 2018
@akien-mga
Copy link
Member

CC @reduz @vnen @willnationsdev

@PetePete1984
Copy link
Contributor

I wouldn't say it's intended behaviour, especially not with an error message that implies cyclic inclusions where there aren't any.
get_class() and is_class() are confusing to me in general, can't comment on that really. The source might solve that mystery in the long run, but so far is has been enough for me.

@ghost
Copy link
Author

ghost commented Aug 27, 2018

@vnen Would this be something that is in need of a clearer error message?

@PetePete1984 Yes, is has been an amazing thing now in 3.1. Though I did come across a situation where I would've liked to use the new match like match custom_thing.get_class() and compare against several strings. But nothing critical, just a readability and less typing thing.

@willnationsdev
Copy link
Contributor

@avencherus You might be able to get away with:

match custom_thing.get_script():
    <typename>: stuff # script classes
    <typename>: stuff
    _: match custom_thing.get_class():
        "Node": stuff # engine classes

@vnen
Copy link
Member

vnen commented Aug 27, 2018

I'm not sure why it's giving this error, I'll have to investigate.

@ghost
Copy link
Author

ghost commented Aug 27, 2018

@willnationsdev Not entirely sure what the check for from get_script() part, for me at least, it returns a GDScript object [GDScript:1057].

@vnen Thanks vnen. X) Some other weird things seem to be going on when I went back to try out these suggestions to see if I could give you some better information. I'm going to pull and rebuild and fiddle with it in a little bit. See if there is anything else about it to add. I may have bumped into a different issue as well along the way.

@willnationsdev
Copy link
Contributor

@avencherus the "class names" of script classes are global variables that reference the script. So...

class_name CustomType
match get_script():
    CustomType:
        print("I am a CustomType!") # should print

@ghost
Copy link
Author

ghost commented Aug 27, 2018

@willnationsdev Oh neat, I see now. I wasn't expecting class_name to work like that, or the custom type to be available to the match pattern . Thanks for sharing this tip Will.

@ghost
Copy link
Author

ghost commented Aug 27, 2018

@vnen Yes, some additional wrinkles to the story. Let me see how well I can articulate them.

Firstly, Pete's solution works, when a base class is given a class_name all appears fine. What I did notice is that going back to the base class and pulling out the class_name CustomBase that I added, it half works. It no longer appears in the node list when adding nodes, but is will work, and no strange errors.

Then to confirm this, I went to recreate the steps of creating a base2.gd -> custom2.gd extended script. Then inside custom.gd use class_name CustomClass2, but no class naming in the base class.

Another issue appeared. The node list had the first one, but listed with an icon that said "Error"

godot master_2018-08-27_20-01-26

So I modified the first base class so they both used class_name. But got odd results again.

Not everything is showing up in the node list, and then the is keyword only returned true for one of them. Though in this screenshot you can see the node list recognizes CustomClass2, yet in the code behind it, is CustomClass2 is returning false.

godot master_2018-08-27_20-05-11

It seems like lot of things are getting switched up or are lingering about behind the scenes. Hard to quite predict what it may be doing with my limited knowledge.

Hope that helps. X)

@bfishman
Copy link

bfishman commented Aug 28, 2018

edit disregard this comment. I hadn't realized that class_name is not implemented in 3.0.6, this issue refers to a 3,1 build anyway.

class_name seems to be broken in 3.0.6. I will make a separate issue, but:

image

@willnationsdev
Copy link
Contributor

@bfishman script classes are not a feature in 3.0.x. That is why it is not working.

@willnationsdev
Copy link
Contributor

willnationsdev commented Aug 28, 2018

I just tried the following and each version worked. All script classes showed up in the CreateDialog, the scripts compiled fine, the scene ran successfully, etc.

With a scene hierarchy like this:

  • Node (base.gd)
    • Node (derived.gd)

Script:

# res://base.gd
extends Node
class_name BaseNode
func _ready():
    print("base")

# res://derived.gd
extends BaseNode
extends "base.gd" # I tried both of these 'extends' lines and they each worked
class_name DerivedNode # I tried with and without this line
func _ready():
    print("derived")

Every time I ran the test, it printed

base
derived
base

...as it should.

My suggestion would be to try clearing your editor's file cache (if perhaps there were changes in the way the file cache was stored in the midst of your programming / updating the engine, that could somehow mess things up until you clear it away).

@ghost
Copy link
Author

ghost commented Aug 28, 2018

@willnationsdev Thanks for looking at, I'll try this out. Is this the files from AppData/Roaming folder? I'll go ahead and clear the whole thing for now. If so, wondering if there is something more specific that I should delete?

@willnationsdev
Copy link
Contributor

@avencherus I'm not quite sure. I just recall @karroffel mentioning something about it when I added some fixes to the script class icon stuff earlier.

@AlexHolly
Copy link
Contributor

Tested on 1093c0f
I don't see any error but it is not showing the Node in "Create new Node" Dialog.

@Oranjoose
Copy link

Oranjoose commented Sep 10, 2018

I'm also getting the error shown in the original post (Windows x64 Godot 3.1 alpha):

image

However, it happens in a different context. I have 3 scripts which each have a different class_name, and each of them extend Node2D. The error does not occur unless one of the scripts references it with the is operator. For example if some_var is Incomer. The weird thing though is that if the same script references the other class_name, there's no problem. And if the other script references the class_name in question, no problem either.

So there's a ClassA, ClassB, ClassC. If ClassC references ClassA with the is operator, then error at load time. If ClassC references ClassB, no error. If ClassB references ClassA, no error.

None of the scripts are in the same scene, and all of the nodes are siblings (no ancestor relationship).

@ghost
Copy link
Author

ghost commented Sep 11, 2018

@AlexHolly Same situation with me now as well. The error seems to be gone, but yes, classes won't appear in the add node window when not having a base script that also uses class_name.

This is in alpha 3.1 c320d93

@kakoeimon
Copy link

A small example by using "is". The same thing can happen if you use "as".
This was mentioned by @Oranjoose too.
cycle.zip

@vnen
Copy link
Member

vnen commented Sep 14, 2018

None of the scripts are in the same scene, and all of the nodes are siblings (no ancestor relationship).

This has nothing to do with the nodes in the scene. Cycles happens in the script themselves.

For now it's quite easy to get cycles when using is and as and if it happens it's not really a bug, it's the way it is. I'll try to improve this but not for 3.1. The original report is a bug because there are no actual cycles in the code.

@Piet-G
Copy link
Contributor

Piet-G commented Oct 13, 2018

I couldn't reproduce the error in 451e5fd as well. The node didn't show up in the create window tho.

@willnationsdev
Copy link
Contributor

I can confirm that this Issue is not present in 0dbe014 (the latest master I pulled). We can probably close this Issue.

Also, I was able to make it so that the script classes that extend scripts still show up in the CreateDialog which was still a bug. PR is linked above.

@ghost
Copy link
Author

ghost commented Oct 16, 2018

@willnationsdev Thanks Will. X) I have been bumping into various issues with class_name, but haven't been able to recreate this one. They have be terribly hard to recreate, but I have since stopped using them.

I have noticed sometimes vague and unrelated errors can appear in a script, because of errors in a script that is being referenced. They become more meaningful when opening the culprit script, and when fixed these other ones clear out as well.

I will re-tailor the topic of the post though, because this cyclic error does seem to still be happening in odd situations and is elusive when trying to recreate it. So there is probably still something to it, I'll leave it open for vnen.

@nathanfranke
Copy link
Contributor

I would like to correct myself, this is still reproducible in 4.0.

image

Test6.zip

@sttifer
Copy link

sttifer commented Apr 9, 2021

This will only be fixed in Godot 4.0?

@nathanfranke
Copy link
Contributor

nathanfranke commented Apr 9, 2021

This will only be fixed in Godot 4.0?

From the lead maintainers, unlikely to be backported to 3.x, since resources are allocated to 4.0. Anyone is free to open a pull request though.

@juanitogan
Copy link
Contributor

  • This one is probably what makes it hard to repro: I have to be in the middle of editing a file and solving other errors. After I get the cyclic error, I just restart Godot and the error usually goes away if the other script problems (like syntax errors) have been solved.

Not only this^^ but sometimes restarting Godot doesn't work if you've gone in deeper than a couple files. And, occasionally, a restart is not needed if I do dummy edits on the files in cyclic lock, in just the right order. What I am currently having to do while refactoring and breaking several files at once is temporarily downgrade the var and parameter typing to just Object until everything is working again. And, then, put the correct typing back in, and then restart Godot. Working like this is brutal and I have a hard time recommending Godot until it is fixed. Okay, if you don't need to code much, Godot is great. But, once you start classing stuff and renaming classes, forget about it. Classes don't "just work" in Godot. They are a constant fight.

So, regardless of whatever parts of this are being fixed in 4.0, it would be nice to at least be able to force a re-analysis of all dependencies in 3.x (or do it more often behind the scenes) so that we don't have to restart or resort to other trickery all the time just to get the parts that do work.... working again.

@auralluring
Copy link

  • This one is probably what makes it hard to repro: I have to be in the middle of editing a file and solving other errors. After I get the cyclic error, I just restart Godot and the error usually goes away if the other script problems (like syntax errors) have been solved.

Not only this^^ but sometimes restarting Godot doesn't work if you've gone in deeper than a couple files. And, occasionally, a restart is not needed if I do dummy edits on the files in cyclic lock, in just the right order. What I am currently having to do while refactoring and breaking several files at once is temporarily downgrade the var and parameter typing to just Object until everything is working again. And, then, put the correct typing back in, and then restart Godot. Working like this is brutal and I have a hard time recommending Godot until it is fixed. Okay, if you don't need to code much, Godot is great. But, once you start classing stuff and renaming classes, forget about it. Classes don't "just work" in Godot. They are a constant fight.

So, regardless of whatever parts of this are being fixed in 4.0, it would be nice to at least be able to force a re-analysis of all dependencies in 3.x (or do it more often behind the scenes) so that we don't have to restart or resort to other trickery all the time just to get the parts that do work.... working again.

The thing that I just started doing to try and get around this is whenever I have a class that frequently refers to other custom classes and those classes refer to it, is to have a dummy class that the custom class inherits from. Then whenever I need to refer to it for static typing or whatever, I do it against the dummy class instead. It makes it a bit strange in the editor and you have to make sure not to actually use the dummy class to do anything or have multiple classes inherit from it, but it does seem to prevent the cyclic errors for the most part (although I do sometimes get it with autoload scripts, but those are easier to fix). There are obviously downsides, like not having full autocompletion and the code being less readable, but it's better than constantly grappling with the classes and trying not to trigger the cyclic error.

@aki-cat
Copy link

aki-cat commented Oct 18, 2021

@HorseLuvver that is called an abstract class/interface, and it should work with autocomplete if you create dummy (virtual) empty methods just to declare its fields. These are basic concepts of Object Oriented Programming and are a feature of classes in general programming.

However, this does not cover all cases of circular reference. It does help to some extent and I have been doing so for some time now. It is good, albeit tedius, practice.

@MikeSchulze
Copy link

👍

@rcorre
Copy link
Contributor

rcorre commented Nov 26, 2022

In 3.5.1, it seems sometimes is will cause a cycle when as will not:

extends Spatial
class_name Thing

func _ready():
	var t := self as Thing # allowed
	assert(self is Thing) # Parser Error: Using own name in class file is not allowed (creates a cyclic reference)

That seems like a bug, but also a potential workaround (for some use cases) until 4.0?
This doesn't seem to be one of the cases that goes away after a reload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment