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

[4.2] GDScript: Lambda hotswap fixes #86860

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

rune-scape
Copy link
Contributor

cherrypicked #86569 for 4.2 :)

@rune-scape rune-scape requested a review from a team as a code owner January 6, 2024 01:43
@dalexeev dalexeev added this to the 4.2 milestone Jan 6, 2024
@AThousandShips AThousandShips changed the title GDScript: Lambda hotswap fixes [4.2] GDScript: Lambda hotswap fixes Jan 6, 2024
AThousandShips
AThousandShips previously approved these changes Jan 6, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Jan 6, 2024

Normally when something is easily done with git cherry-pick we leave it to the production team to handle, unsure if this was trivial

Will need a decision from the gdscript team and see if it doesn't cause any unforeseen issues before merging but code looks good, but would wait a while to see if it introduced any issues 🙂

@rune-scape
Copy link
Contributor Author

ah 👍 ya it was trivial, not much of the surrounding code changed , didn't know 👍

@AThousandShips
Copy link
Member

Then future suggestion would be to ask on the PR if it can be cheery picked

@akien-mga
Copy link
Member

@rune-scape Could you help us evaluate how risky this PR is, i.e. does it have potential to introduce regressions and are the issues it fixes worth it?

For stable branches we're usually quite conservative with what we cherry-pick, and this code specifically had a number of critical regressions in the phase leading to 4.2-stable. It's not clear whether users are affected by the issues #86569 fixes in the wild (no bug report), so if it has potential to instead break some functionality, I'd opt for not cherry-picking the PR (and let it get tested thoroughly in the 4.3 dev phase).

@akien-mga
Copy link
Member

CC @adamscott @RandomShaper

@AThousandShips AThousandShips dismissed their stale review January 15, 2024 13:49

More complicated

@RandomShaper
Copy link
Member

Given that I won't be very aware of how this turns out in the wild, all I can say is I agree with not cherry-picking at least until 4.3 has been thoroughly tested and also it's maybe not worth the risk if there are no reported issues in 4.2, anyway. I wouldn't oppose, though, regardless no reports, but, yes, let's see if the current approach crystalizes into stability in the wild.

@rune-scape
Copy link
Contributor Author

technically not a critical bug, but the way that #81628 works is fundementally broken, the the PRs that followed didnt fix the fact that it invalidates basically all lambdas unless theres only 1 script with lambdas, making it safer than nothing ig
i can make an issue demonstrating this for 4.2,
the original code is convoluted and has 2 mutexes to lock in order to add a func ptr, which could deadlock if not done properly,,, thats what most of the following PRs helped with, but they also made the code even harder to follow and inspect
this code only uses 1 mutex very simply, i think its much safer in that regard

@akien-mga
Copy link
Member

akien-mga commented Jan 18, 2024

The thing is, if it's breaking hot reloading, it's not a big deal, it's just an editor time inconvenience. What it addressed however was runtime crashes which were critical. I want to make sure we don't re-introduce runtime issues for the sake of improving editor lambda editing usability.

@rune-scape
Copy link
Contributor Author

ok i understand, was giving my input and maybe was distracted from how important its stability is,
i do think the way this code is written is much safer than what is in 4.2 fwiw, but not tested nearly as much

@akien-mga
Copy link
Member

Let's give it a go then and see what happens with 4.2.2 RC 1 tests.

@akien-mga akien-mga merged commit 1cb7d6f into godotengine:4.2 Jan 18, 2024
15 checks passed
@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.

5 participants