hpi-swa-lab / godot-pronto

https://hpi-swa-lab.github.io/godot-pronto/
18 stars 4 forks source link

Deleting nodes with incoming connections causes errors #200

Closed jgrenda closed 9 months ago

jgrenda commented 11 months ago

Recording 2023-12-02 at 14 28 20

Deleting nodes with incoming connections causes error spam in the console. The error: res://addons/pronto/helpers/Connection.gd:270 - Assertion failed. The referenced line in Connection.gd:270: assert(is_expression()) is_expression(): func is_expression() -> bool: return expression != null

leogeier commented 10 months ago

I am having difficulties reproducing this. Can you provide a minimal reproduction project?

JulianEgbert commented 10 months ago

Example

We tested this with a simple Example: Add a KeyBehavior and a CodeBehavior to the scene. Connect the KeyBehavior to the CodeBehavior (with any event from the Key) and select execute in the CodeBehavior. If you now delete the CodeBehavior, you get the mentioned error.

Cause of this problem

The cause seems to be in is_valid() in Connection.gd: A node is considered valid iff not not to.is_empty() (meaning to is empty?!) or the target node is null. This is incorrect because we can have a statement connection from a node that has no to. Also, we think the function is_target() causes an Issue here: This should rather be renamed to has_target() since it only checks if the connection has a node in to (meaning it has a target, rather than beeing it?!). In my opinion we should NOT differentiate between a double clicked self statementand a connection to the node itself (since this only adds to to the scope of the statement) with <statement(s)>.

func is_target() -> bool:
    return not to.is_empty()

func is_valid(from: Node):
    return not is_target() or from.get_node_or_null(to) != null

Since this effects a few other positions in Connections.gd we didn't perform any changes yet and are waiting for feedback. @leogeier @tom95

JulianEgbert commented 10 months ago

In the merge request we now also added that connections to a node itself always have the node as a receiver (to-property). Specifically this is the case for connections that were added by double clicking a trigger.

Please review the MR (#202)

leogeier commented 10 months ago

I am still unable to reproduce the error on macOS 14.1.2, Godot 4.2. Which platforms are you working on?

JulianEgbert commented 10 months ago

I tested this on Windows with Godot v4.2.beta2.official [f8818f85e].

JulianEgbert commented 10 months ago

Switching to v4.2.1.stable.official [b09f793f5] it works fine for me too!

jgrenda commented 10 months ago

Can confirm that switching to the latest stable release fixed this for me as well (v4.2.1-stable)