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

Fix unable to disconnect signal in Editor once created #69661

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Dec 6, 2022

Fixes #69348.

#66665 implementation was a bit too stupid and didn't account all cases. Most importantly, signals that had yet to be saved.

To solve this in the least chaotic, slow and intrusive way, I basically gave up. This PR adds a CONNECT_INHERITED flag to connections, only available in editor builds. This flag denotes that the signal has been inherited from a previous Scene in the instancing hierarchy. Signals deemed to be inherited cannot be edited or deleted in the editor.

@akien-mga akien-mga requested a review from KoBeWi December 6, 2022 13:31
@akien-mga akien-mga added this to the 4.0 milestone Dec 6, 2022
@Mickeon Mickeon force-pushed the fix-editor-cannot-disconnect-signal branch from 740fb70 to 37a0a76 Compare December 6, 2022 15:39
@KoBeWi
Copy link
Member

KoBeWi commented Dec 6, 2022

Seems like this PR makes it possible to disconnect foreign connections again...

@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 6, 2022

Alright, back to work.

@Mickeon Mickeon marked this pull request as draft December 6, 2022 16:49
@Mickeon Mickeon force-pushed the fix-editor-cannot-disconnect-signal branch from 37a0a76 to 0b9ce00 Compare December 7, 2022 17:53
@Mickeon Mickeon marked this pull request as ready for review December 7, 2022 17:53
@Mickeon Mickeon requested a review from a team as a code owner December 7, 2022 17:53
@Mickeon Mickeon requested a review from akien-mga December 7, 2022 17:55
@Mickeon Mickeon force-pushed the fix-editor-cannot-disconnect-signal branch 2 times, most recently from ab132b4 to ea93098 Compare December 7, 2022 18:02
core/object/object.h Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the fix-editor-cannot-disconnect-signal branch from ea93098 to f08cfaa Compare December 7, 2022 20:28
@@ -450,7 +450,11 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
callable = callable.bindp(argptrs, binds.size());
}

#ifdef TOOLS_ENABLED
cfrom->connect(snames[c.signal], callable, CONNECT_PERSIST | c.flags | (p_edit_state == GEN_EDIT_STATE_MAIN ? 0 : CONNECT_INHERITED));
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this possibly be useful at runtime too?

Copy link
Member

Choose a reason for hiding this comment

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

It could be useful if someone uses custom scene serialization, but it's rather corner-case.

@@ -450,7 +450,11 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
callable = callable.bindp(argptrs, binds.size());
}

#ifdef TOOLS_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

Would remove this ifdef too, probably not worth it

Copy link
Member

Choose a reason for hiding this comment

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

just to clarify, PERSIST is also editor only, so no point of having one with ifdef and the other without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know... CONNECT_PERSIST can technically also be used in release builds when building PackedScenes at run-time

Adds a CONNECT_INHERITED flag to connections, only available in editor builds. This flag denotes that the signal has been inherited from a previous Scene in the instancing hierarchy.
@Mickeon Mickeon force-pushed the fix-editor-cannot-disconnect-signal branch from f08cfaa to 885f2a4 Compare December 8, 2022 12:37
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.

There might be a case for actually exposing the new enum value to be consistent, since users may want to use it in editor plugins?

Otherwise seems good to me.

@akien-mga akien-mga merged commit 2d02cb6 into godotengine:master Dec 9, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the fix-editor-cannot-disconnect-signal branch February 11, 2023 23:04
@KoBeWi
Copy link
Member

KoBeWi commented Apr 7, 2023

Seems like discarding instancing (using the Make Local option) doesn't remove the flag and the connections cannot be disconnected at all.

EDIT:
Yep, the flag is incorrectly stored in the local scene. There is no way to remove it other than manually editing the tscn file.
I'll open an issue I guess.

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.

Cannot disconnect signal with context menu once created
4 participants