-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
C#: Fix assert and assembly unloading failure when using custom script parameters with a default value #81541
Conversation
For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info. |
Thanks for the heads-up! I don't do much git/github work so I didn't realize that was the case. |
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.
Not a maintainer, but here's some things I noticed while skimming through the code. Haven't tested but it looks good, otherwise!
modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
Outdated
Show resolved
Hide resolved
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.
Just got around to testing this! Everything seems to be working correctly and the code looks good to me. However, because the root cause of this issue is script loading order (which afaik is really hard to control, I was very rarely able to reproduce the error prior to this PR), I think it would be good to get someone else familiar with the .NET side of things to also test this before merging. Since I'm not actually a part of the Godot team, you need would someone from the .NET team to approve these changes anyway, so I'm sure that'll happen whenever they get around to it.
Also, this completely slipped my mind previously, but since you are a new contributor, make sure to read CONTRIBUTING.md and the contributing documentation if you haven't already.
You will also need to squash the commits before this PR can be merged. You can find more info about squashing commits here.
Feel free to reach out in the development chat if you need help.
… using a script that contains a default constructed parameter referencing a custom script: - The issue is one of dependency between assembly reloading creating and reloading the script on demand at the moment of default constructing the owner script and then trying to register it again when loading the file containing the script used by the default constructed parameter; - The fix relies on checking whether the script has already been registered during the assembly reload and uses it to reload dependent objects instead of trying to register the original script again;
7950a64
to
b534c05
Compare
Thanks for the review and resources. The PR should hopefully be set up correctly now. :) |
Please merge this @godotengine this would resolve several issues I'm having, detailed in #80175 |
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.
Couple of questions, because I'm never sure of all the implications when dealing with the reloading logic. Overall, I think it'd be an interesting fix to get in, this issue is quite problematic for a lot of people.
String script_path = scr->get_path(); | ||
|
||
if (GDMonoCache::godot_api_cache_updated && GDMonoCache::managed_callbacks.ScriptManagerBridge_GetExistingScriptBridgeForPath(&script_path, &existing_script)) { | ||
existing_script->pending_reload_instances = scr->pending_reload_instances; |
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.
Wondering if those should be merged instead of replaced in this case.
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.
Iirc, the existing script would not be part of the reload as it was created on demand during another script's reload, so it would not have any instances to merge.
if (GDMonoCache::godot_api_cache_updated && GDMonoCache::managed_callbacks.ScriptManagerBridge_GetExistingScriptBridgeForPath(&script_path, &existing_script)) { | ||
existing_script->pending_reload_instances = scr->pending_reload_instances; | ||
existing_script->pending_reload_state = scr->pending_reload_state; | ||
existing_script->_take_over_path(script_path); |
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.
Is this necessary here? I think it'd be an issue if we end up with CSharpScript
s in the bimap, but don't have the right path on the resource already?
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.
I believe the important part here is that we're taking over the path from the other script, since we otherwise have both of them pointing to the same path which I think was leading to asserts.
I've been unable to reproduce the issue, so I'm trying to understand what's going on here. From what I understand reading the PR description, the issue occurs when a script is reloaded twice? It should be possible to reload a script multiple times without causing issues because when the reload is not invalidated the reload request should be ignored. If that's not happening then maybe something else is going on here. Let's say that we have two scripts class ScriptA : Resource { }
class ScriptB : Node
{
[Export]
public ScriptA Resource { get; set } = new();
} If we reload Later when It should not try to add the same script to the map twice, and there can't be multiple instances for the same Can anyone who can reproduce this issue help me understand where this process fails? |
Hey @raulsntos, thanks for taking a look! I have an MRP attached to the linked issue - all you should need to do in order to reproduce the bug is to open it and build from godot or an external editor, which will lead to the following error: It's been a while since I worked on this issue, and the order of things is a bit complicated in the first place so I hope I don't get it wrong here, but I belive the gist of it is was this: Hopefully that helps at least a bit, but I might be off on the details as I haven't looked into this issue in a long time (since around the time I opened this PR to be honest). |
What my understanding is, after a bit of digging.
|
Thank you both for helping me understand the issue, I've been able to reproduce it now. I think #87838 is a better solution so I'll close this PR as superseded. Thanks for the contribution nonetheless! |
Fixes #80175
When an assemby reload gets triggered and a user script is loaded that uses another user script as a parameter with a default value, that script will be loaded on the spot and registered to the
ScriptManagerBridge
ScriptTypeBiMap
.After the assembly reload is done, the script gets reloaded as well and an attempt is made to add it to the map, which will trigger a double insertion assert when trying to add the type to
_typeScriptMap
, but the pointer does gets added to_scriptTypeMap
which seems to then forever prevent assembly unloading since the pointer lingers in the dictionary.The fix is fairly naive since I'm not all that familiar with the codebase, so feel free to see this PR as more of an attempt to bring more attention to the issue in case a more comprehensive solution is necessary here (the assembly reloading error is a big workflow hamper for me as it requires a restart after any code change, or to work around the issue by not using default values which is something I'd like to avoid if possible).
MRPs can be found in the original issue - tested that reloading worked correctly without editor errors on both.