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

Ensure binds are duplicated with Node signals #75382

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 27, 2023

Also added a check to ensure the function has a method name, doesn't fully prevent pointer binds from ending up attempting to duplicate (it would be an error to have such a signal as persists anyway), could make a more elaborate change involving adding a rebind function to Callable but think it is outside the scope of this

Fixes #75105

@KoBeWi
Copy link
Member

KoBeWi commented Aug 17, 2023

Unbinds have the same problem.

@AThousandShips
Copy link
Member Author

Will look into it as well, thank you

@AThousandShips
Copy link
Member Author

Update:
Now updates unbinds as well

To do:

  • Should it respect CONNECT_REFERENCE_COUNTED and skip the check for already bound in the target?
  • Should the variant array be duplicated?

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 30, 2023
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.

Should it respect CONNECT_REFERENCE_COUNTED and skip the check for already bound in the target?

I think the method is always called on freshly duplicated nodes, so it doesn't matter.

Should the variant array be duplicated?

If you mean the array from get_bound_arguments(), the method always returns a new Array.

@AThousandShips
Copy link
Member Author

Meant if the duplicated node has multiple signals connected, this will connect only the first one, but can be discussed in a follow-up

@akien-mga akien-mga merged commit 39b1d86 into godotengine:master Sep 1, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips deleted the duplicate_bind branch September 1, 2023 14:11
@AThousandShips AThousandShips added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Nov 3, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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.

Signal binds not duplicated when duplicating node
4 participants