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

Do not allow editing Scene-inherited signal connections #66665

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 30, 2022

Closes #12373.

Editing inherited signal connections has never actually really worked, in practice.
In fact, saving and reopening the Scene makes all inherited connections appear again, as if no change has even happened. Very strange to let the user touch them, in the first place. It really has no effect. Nor it probably should, as it would lead to an unorganised workflow.

This PR makes it so that signal connections from an inherited Scene can no longer be modified in any way in the Editor.
Inherited connections are displayed in warning yellow (same as inherited Nodes) and they cannot be edited or removed.

Showcase

Likewise, "Disconnect All" will not disconnect inherited connections, and is disabled if no connection can be disconnected.
image

@KoBeWi KoBeWi added this to the 4.0 milestone Sep 30, 2022
@Mickeon Mickeon force-pushed the editor-do-not-edit-inherited-signals branch 3 times, most recently from 1b529db to 7d05ead Compare September 30, 2022 15:27
@akien-mga akien-mga requested a review from KoBeWi October 13, 2022 15:28
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea seems fine to me, could use a more in depth review as I just gave it a quick look.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 13, 2022

Found a bug. Connecting an instance blocks disconnecting until reload.

godot.windows.editor.dev.x86_64_vouq1whwx5.mp4

@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 13, 2022

Connecting an instance blocks disconnecting until reload.

Hmm... It technically makes sense. I wonder how to address this one...

@KoBeWi
Copy link
Member

KoBeWi commented Oct 13, 2022

It's not important tbh, so it can be fixed later (e.g. when someone opens an issue xd)

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, aside from some comment typos and unimportant issue.

Inherited connections are also highlighted with the warning color in the Node dock.
@Mickeon Mickeon force-pushed the editor-do-not-edit-inherited-signals branch from 7d05ead to 6a77563 Compare October 13, 2022 18:13
@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 13, 2022

Reflecting on the bug more, to solve it would mean to change the way direct scene connections are detected, to do a bit of the opposite of what it is currently doing. Which is basically Godot going "Oh, I don't see this signal within the list. Therefore it has to be inherited" but as you showed, it isn't necessarily true.

@akien-mga akien-mga merged commit 7502c80 into godotengine:master Oct 14, 2022
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

it's impossible to disconnect signals in inherited scene.
3 participants