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

Allow cyclic references #460

Closed
nathanfranke opened this issue Feb 10, 2020 · 20 comments
Closed

Allow cyclic references #460

nathanfranke opened this issue Feb 10, 2020 · 20 comments

Comments

@nathanfranke
Copy link
Contributor

nathanfranke commented Feb 10, 2020

Describe the project you are working on:

  • Networking packet system
  • Celestial bodies, script hierarchy

Describe the problem or limitation you are having in your project:

Use Case: Packets

Static constructor wrappers are impossible to make due to Parser Error: Using own name in class file is not allowed (creates a cyclic reference)

Consider the following code

extends Packet
class_name PacketPing

var message: String

static func create(message: String) -> PacketPing:
	var packet: PacketPing = PacketPing.new() # Cyclic reference
	
	packet.message = message
	
	return packet

# We may need multiple initializers that do completely different things
#static func from_bytes(bytes: PoolByteArray) -> PacketPing:

It is impossible to make a static packet constructor wrapper for this since PacketPing refers to itself

Use Case: Scripts in a hierarchy

image

extends Node
class_name CelestialBody

var mass: float
var radius: float

# Check if we are orbiting another CelestialBody
func is_orbiting() -> bool:
	return get_parent() is CelestialBody # Cyclic reference

What I don't understand: Cyclic references work fine in almost every other programming language

Cyclic references that work fine in 5 of the most popular programming languages

C#:

public class A
{
    public static A Create()
    {
        return new A(); // This works fine!
    }
}

Java:

public class A {
    public static A create() {
        return new A(); // This works fine!
    }
}

C++:

class A {
public:
    static A create()
    {
        return A(); // This works fine!
    }
};

JavaScript:

function A() {
		this.create = () => new A(); // This works fine!
}

Python:

class A:
    @classmethod
    def create(cls):
        return A() # This works fine!

PHP: It should be noted that cyclic references actually do not work in PHP


Describe how this feature / enhancement will help you overcome this problem or limitation:
Cyclic references are completely valid for good code. It is needed to make named, static, wrapped constructors.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
GDScript code samples above would run without errors since cyclic references should be valid

Describe implementation detail for your proposal (in code), if possible:
Cyclic references cannot be implemented without changing the scripting engine

If this enhancement will not be used often, can it be worked around with a few lines of script?:
No, right now there is no good way to work around cyclic references. is operator can be worked around by comparing the script with load()

Is there a reason why this should be core and not an add-on in the asset library?:
This can not be worked around with an addon. Cyclic references are a core part of the scripting engine

@Calinou
Copy link
Member

Calinou commented Feb 10, 2020

Related to godotengine/godot#21461. There are plans to solve this in 4.0.

@GameDevLlama
Copy link

I had the same issue yesterday.
One thing for now which can work is:

Overriding the get_class() function and use that to check if itself is of a certain type. Not sure if performance is well for this but at least I could get rid of cyclic reference warnings while keeping most of my types.

@vnen
Copy link
Member

vnen commented Feb 13, 2020

I think this can be closed, since we will solve this anyway and there's already an issue being tracked in the main repository.

@nathanfranke
Copy link
Contributor Author

Isn't that issue about a specific problem related to class_name and is? This proposal is general for all cyclic examples

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Feb 13, 2020

Closing for now, Duplicate of godotengine/godot#21461

@Xrayez
Copy link
Contributor

Xrayez commented Feb 17, 2020

Another instance of (future) cyclic references which can happen while serializing GDScript instances: godotengine/godot#33137 (currently just errors out without crashing at least). 😛

@nathanfranke
Copy link
Contributor Author

@akien-mga Re-open this since we are migrating to proposals?

@vnen
Copy link
Member

vnen commented May 26, 2020

There's no need to reopen, the issue is already tracked in the main repo as a bug.

@nathanfranke
Copy link
Contributor Author

From that issue:

Certain uses of class_name can produce cyclic errors in situations where there are no cyclic references

So isn't that unrelated?

@vnen
Copy link
Member

vnen commented May 27, 2020

@nathanfranke the issue evolved from the initial description to cover all cases. Don't worry, what's in this proposal will also be allowed.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 27, 2020

@vnen I suggest to re-open this proposal to prevent further duplicate proposals, see previous #595, #1629, #1737, even if this is already tracked in the main repository.

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Oct 28, 2020

I agree

Edit: We should still keep godotengine/godot#21461 open since it seems to be a separate bug.

@nathanfranke nathanfranke reopened this Oct 28, 2020
@Calinou Calinou removed the archived label Oct 28, 2020
@cgbeutler
Copy link

cgbeutler commented Aug 16, 2021

Question: Does this mean that prior to 4.0 I need to be careful of circular refs in C# when using Reference-derived types?

@rossunger
Copy link

In case this is helpful for anyone, I've been using a hacky workaround instead of the "is" or "as" keywords...
I compare get_script().get_path() against the filename of the script.
This is the only way I found to stop cyclic dependency errors.
Is there any chance that a get_script_name() function might make it's way into godot 3?
I'm looking forward to switching to godot 4 but until then, gotta use the hacks.

@Shadowblitz16
Copy link

Does 4.0 have this? If so can we get a backport to v3.4.5 along side custom resource exports?

@Calinou
Copy link
Member

Calinou commented Mar 25, 2022

Does 4.0 have this?

It does, but only to a limited extent as there are still some bugs with the current implementation.

If so can we get a backport to v3.4.5 along side custom resource exports?

No, as this change relies on a complete rewrite of GDScript (which is backwards-incompatible).

Also, patch releases don't get any new features to reduce the risk of introducing regressions.

@marcelb
Copy link

marcelb commented Jul 7, 2022

I would love to see this! I'm working on a RogueLike which features randomly generated interconnected rooms, which are basically a linked list or graph.

A Room has 0-3 Doors which reference another room. So of course my cyclic reference is Room->Door->Room. Or more simply Room->Room.

class_name LinkedRoom extends Reference

var doors:Array = [] # Array of Doors
class_name Door extends Reference

var linkedRoom:LinkedRoom

I circumvented the problem with load(), but the code gets less readable and weird. I can fully understand runtime circular instancing problems, but in this case the class that is already loaded shouldn't be loaded again causing recursive loading.

There are some ways to make it work without a linked list, but they are harder to understand and less flexible.

@nathanfranke
Copy link
Contributor Author

Should be resolved as of godotengine/godot#67714

@akien-mga akien-mga added this to the 4.0 milestone Nov 18, 2022
@Calinou Calinou moved this to In Discussion in Godot Proposal Metaverse Nov 19, 2022
@Calinou Calinou moved this from In Discussion to Implemented in Godot Proposal Metaverse Nov 19, 2022
@Lilith-In-Starlight
Copy link

Will this be backported for 3.x?

@YuriSizov
Copy link
Contributor

@Lilith-In-Starlight This is not a change that can be backported. It needs to be redone completely for a 3.x branch, and that's unlikely, unfortunately.

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

No branches or pull requests