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

For v4.3+, switch #[class(tool)] to use Godot's native "runtime class" feature #619

Merged
merged 1 commit into from Feb 21, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Feb 20, 2024

For Godot 4.0, 4.1 and 4.2, gdext emulates a mode where classes' lifecycle methods are not executed in the editor, unless #[class(tool)] is specified or the global editor_run_behavior() is configured.

Starting from Godot 4.3, GDExtension provides this feature natively, dubbed "runtime classes" (formerly "gameplay classes").
This has been implemented in godotengine/godot#82554 and merged a few hours ago.

This PR thus conditionally switches to the native implementation, when API 4.3 is targeted.

I briefly tested the functionality with dodge-the-creeps, but it might be that there are still some bugs, as it's hard to write automated tests for this. Let us know in case things don't work as expected.

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Feb 20, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-619

@Bromeon Bromeon added this pull request to the merge queue Feb 21, 2024
Merged via the queue into master with commit 82903f9 Feb 21, 2024
16 checks passed
@Bromeon Bromeon deleted the feature/runtime-classes branch February 21, 2024 16:42
@kang-sw
Copy link
Contributor

kang-sw commented Feb 25, 2024

Hello, sorry for commenting on closed PR. I'm experiencing panics on any access to singletons (checked from DisplayServer, RenderingServer) from tool classes, after hot reloading in editor. Is this known issue and being fixed from this PR(and from Godot 4.3), otherwise should I rather to open separate issue with MRP?

  • Currently working with Godot 4.2
  • Latest master branch (Feb 25, 2024)

Usually this assertion is hit:

#[inline(always)]
pub unsafe fn class_servers_api() -> &'static ClassServersMethodTable {
    // SAFETY: `get_binding` has the same preconditions as this function.
    let binding = unsafe { get_binding() };

    debug_assert!(
        binding.class_server_method_table.is_initialized(), // <<< THIS ONE!
        "cannot fetch classes; init level 'Servers' not yet loaded"
    );

    // SAFETY: `initialize_class_server_method_table` has been called.
    unsafe { binding.class_server_method_table.get_unchecked() }
}

@Bromeon
Copy link
Member Author

Bromeon commented Feb 25, 2024

That problem has been reported on Discord before, but I don't see the relation to this PR?

@kang-sw
Copy link
Contributor

kang-sw commented Feb 26, 2024

Aha, I was curious if the bug was related to tool class behavior. Thanks for checking it out! I think #582 issue is simillar situation with me where crashing on engine resource access after hot reloading, however, I'm not sure if it's exactly the same issue.

I've just tested another PR which targets to resolve broken hot reloading by reverting some commit(#628, revert UnsafeOnceCell to static mut), it still triggers similar assertion at similar code point.

#[inline(always)]
pub unsafe fn class_servers_api() -> &'static ClassServersMethodTable {
    let table = &unwrap_ref_unchecked(&BINDING).class_server_method_table;
    debug_assert!(
        table.is_some(), // <<< Only method name changed ...
        "cannot fetch classes; init level 'Servers' not yet loaded"
    );

    table.as_ref().unwrap_unchecked()
}

May I create a separate issue about this? This is pretty big blocker for me developing egui plugin for editor plugin purpose. 😢

@Bromeon
Copy link
Member Author

Bromeon commented Feb 26, 2024

Yes sure, feel free to open an issue :)

Maybe also comment on #628. But in its current form I can't merge that one, we should isolate the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants