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

Revert "Merge pull request #581 from lilizoey/fix/static-mut-safety" #628

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Feb 25, 2024

This reverts commit b4a91a6, reversing changes made to 6243fc6.

Hot reloads appear to have been broken by this PR. Putting in a revert to get hot reloading back up and running while the cause is investigated.

…t-safety"

This reverts commit b4a91a6, reversing
changes made to 6243fc6.
@TCROC
Copy link
Contributor Author

TCROC commented Feb 25, 2024

Not really sure why CI is unhappy here.

@kang-sw
Copy link
Contributor

kang-sw commented Feb 26, 2024

It seems this PR incurs compile error on godot-core\src\private.rs:45:26 which utilizes new API introduced the commit being reverted by this one.

    global_config.tool_only_in_editor //.
        && global_config.is_editor_or_init(is_editor)
        //               ^^^ This one

Removing that line compilation itself succeeds. (Of course idk if it works or not ..)

@TCROC
Copy link
Contributor Author

TCROC commented Feb 26, 2024

Thanks for the heads up! I'll try it in the morning and see if it fixes it

@Bromeon Bromeon added bug c: ffi Low-level components and interaction with GDExtension API labels Feb 26, 2024
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

I can't just revert all those changes to fix one issue. They improve on several other things, like thread safety.

We should isolate what causes the issue and fix that specifically.

In this case, IIRC the assumption that some global initialization would only be called once was violated -- it's called multiple timed in hot reloading, which panics? If so, we should hook the hot reload notification to a flag, which is then considered on reload.

@StatisMike
Copy link
Contributor

Full agreement with @Bromeon. @TCROC: Does enabling the experimental-threads feature fixes your issue ATM, as it does in #610 and #597?

If it does, we should maybe communicate better their (at least temporary) connection, and it should be used until we pinpoint the underlying issue at hand.

@TCROC
Copy link
Contributor Author

TCROC commented Feb 26, 2024

@StatisMike This does indeed fix it! :) I prefer this as a workaround rather than a revert. I'll close this PR and open an issue to track it instead with a summary of our findings.

@TCROC TCROC closed this Feb 26, 2024
@TCROC TCROC deleted the revert-unsafe-oncecell-to-fix-hot-reload branch February 26, 2024 12:47
@Bromeon
Copy link
Member

Bromeon commented Feb 26, 2024

@TCROC this is already tracked in #629, please don't open a new one.

@TCROC
Copy link
Contributor Author

TCROC commented Feb 26, 2024

Sounds good. I won't :)

@Bromeon
Copy link
Member

Bromeon commented Mar 30, 2024

Fixed in #653.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: ffi Low-level components and interaction with GDExtension API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants