-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
[FIX] [WASM-Export] Unable to use multiple rust GDExtensions at the same time #973
base: master
Are you sure you want to change the base?
Conversation
For some reason iterating through all the class names made them "correct", as the runtime previously complained that it's trying to register classes with the same name. It runs with a ton of errors about double registering methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this pull request!
I'm not sure if we should just add a flag "already loaded" to circumvent the current checks that ensure it's not loaded twice. This defeats their presence, and we could as well have only a global flag. With this approach we risk that accidental double initializations are no longer visible. This is e.g. what happened with Linux hot reload (fixed in #656), and the current logic would hide such bugs.
So I'm wondering if being a dependency is not something that should be explicitly declared by the user, and the library would verify that the assumptions really hold. In #968 (comment), I suggested a macro attribute:
#[gdextension(dependency)]
unsafe impl ExtensionLibrary for ClientExtension {}
Did you have a chance to experiment with that? (Totally fine if not 🙂 )
The behavior you found with ClassName
is interesting. In the quoted code
let string_name = entry
.godot_str
.get_or_init(|| entry.rust_str.to_string_name());
the StringName
is lazy-initialized. This is necessary because the builtin type StringName
itself isn't yet loaded at the time the classes are collected by the "plugin" system. Which is also why ClassName::new_cached()
takes Rust standard String
.
You mention:
So there was a Node
TestNodeFromClient
and a NodeTestNodeFromShared
, but both element'sClassPlugin
was referring to the nameTestNodeFromClient
How and when (at which initialization stage) did you print this? Is this behavior also there if you use ClassName::to_cow_str()
, which doesn't rely on Godot functionality?
We have two global variables here. Do you know if they only exist once in memory and hold all the classes?
gdext/godot-core/src/meta/class_name.rs
Lines 26 to 28 in 3c56b3f
// First element (index 0) is always the empty string name, which is used for "no class". | |
static CLASS_NAMES: Global<Vec<ClassNameEntry>> = Global::new(|| vec![ClassNameEntry::none()]); | |
static DYNAMIC_INDEX_BY_CLASS_TYPE: Global<HashMap<TypeId, u16>> = Global::default(); |
(Sorry for the interrogation, I'm trying to get to the root of where this duplication/overlapping happens 😀 )
Hi @Bromeon, I'm repeating myself again and again, but thank you for the great response! Very in-depth and insightful.
I did experiment with this (not with an attribute macro but just some random hacking with a boolean flag) however what I noticed is that the loading of plugins seemed not always consistent, so stopped trying in that direction. While experimenting, I noticed that sometimes the extensions in Sometimes the order was: 1. Shared, 2. Client ("hacked" as dependency), and other times it was 1. Client (dependency), 2. Shared. If we could find a way to make sure the "dependency" is in fact always loaded after the "non-dependents", this would could be fruitful!
I think I have a saved log somewhere. The log
In the above, the line let string_name = entry
.godot_str
.get_or_init(|| entry.rust_str.to_string_name()); The error did not happen, so validation happened correctly. Please keep in mind I know a bit more about the library, but still nothing at all. I just happened to want to print the string names on start up, to verify that indeed I'm not sure how to continue at the moment, however I'm hellbent on getting it to work now. I've crawled so deep into this library at the moment, it'd be a waste not to fix it properly. Do you have any recommendations for the consistency of loading the plugins? I'm convinced that the |
I'm not sure how exactly Godot determines the order of GDExtensions, or if there is a deterministic way to do so. This But in your case, that means you need to register the 3 parts as individual extensions? If the Thanks for the debugging insights! I'll need to analyze that a bit more in-depth once I get some time. |
Yeah, I agree the My case is that I'd like to have the Just throwing some ideas around, but maybe I should go a step back and research why the three extensions work fine on my computer, a Linux box, and not on the web. Because Godot is already succesfully loading all projects (I can use them fine in the editor, and running the game is also working fine.) I have a bit of experience using I know I'm asking a lot of questions, please excuse me for that. I'm willing to take it on, but I'd need your support so that it's actually an idea you'd like to see merged :) |
Yeah, I think making the behavior more consistent and intuitive would be great! Myself, I'm not a Wasm expert, and it's one of the areas where I can't devote as much time as I'd like to (several contributors like @PgBiel have done a tremendous job here). That said, one thing that's somewhat important to me is that we try to understand why something happens. It's often easy to patch something to get the expected behavior right now, but without understanding the underlying problem, another issue will likely come up again in the future. And if the original author of that patch is no longer actively contributing, such code is now even harder to understand. Of course, there are situations where other tools (emscripten, Godot, browsers, ...) have bugs that just need to be worked around. |
It runs...!
When I was running it past engine initialization, I encountered an error that registered the two seperate plugins as though they were the same. So there was a Node
TestNodeFromClient
and a NodeTestNodeFromShared
, but both element'sClassPlugin
was referring to the nameTestNodeFromClient
. I thought this was a strange issue and investigated. After changing some code to let the runtime print me a list of registered class names, I noticed that the error was gone. So for some strange reason iterating through all the class names made them "correct", as the runtime previously complained that it's trying to register classes with the same name. I suspect this is due to the linewhich might have corrected the previous wrongly set class name? I'm not really sure. (Edit: Could #834 have to do something with it?)
For now, it runs with a ton of errors about double registering methods; this is probably due to the runtime registering the methods twice. It does run however, and it seems to give the correct output.
Please do not actually merge this, as it isn't pretty. I'm looking for ways to improve this.
I suspect there needs to be a bigger refactor to make this production worthy code. It seems as though there's only one runtime in WASM, when on Linux I have no issues with the same code. This means that even though the runtime in WASM shares it's memory (as I could use a
static
to see if the iniitalization code already ran), however it runsauto_register_classes
for every plugin. I'm not sure how to mitigate this really...