Update connection comparison operator to compare connection sources based on instance ID instead of by pointer #52885
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ased on instance ID instead of by pointer
akien-mga
approved these changes
Sep 21, 2021
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
|
Cherry-picked for 3.3.4. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar change to the recently merged PR 52493, which I came across while investigating the unpredictable signal ordering detailed in Issue 35084. PR 52493 fixed comparisons of
targetbetween 2 connections, but there is a similar issue comparingsourcebetween connections. The current setup compares pointers which results in inconsistent comparison outputs for connections with different sources.As far as I can tell, there's not currently much impact from how connection sources get compared -- I believe the only current caller of
Object::Connection::operator<isSceneState::_parse_connectionsviaconns.sorton line 693 ofpacked_scene.cppwhich gets called while saving scenes, and that method already builds up a list connections one node at a time, sosourceis always identical between any connections that get compared. So after some testing it doesn't seem like this is actually a cause of the unpredictable signal order described in Issue 35084. But it does seem like the comparison logic here is inaccurate and could potentially cause other problems in the future.Tested some basic operations on my local build of Godot after this change and all seemed good. I think this connection comparison method would be a good candidate for some unit tests, but I'm not too familiar with how testing works in Godot so I haven't included them for now.