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

Prevent dispose of Static StringName #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scgm0
Copy link
Contributor

@scgm0 scgm0 commented Sep 4, 2024

Try fix #20 again

@scgm0 scgm0 force-pushed the Prevent-repeated-instantiation-of-static-StringName branch 5 times, most recently from 5b385c6 to 29ee39c Compare September 4, 2024 21:01
@takethebait
Copy link

While testing, when quitting a scene I no longer get the unreferenced string errors but I now get a couple of orphaned stringNames:

Orphan StringName: _setup_local_to_scene (static: 0, total: 50)
Orphan StringName: OS (static: 2, total: 3)
Orphan StringName: Node3D (static: 2, total: 3)
Orphan StringName: InputEventMouseMotion (static: 2, total: 52)
Orphan StringName: _get_rid (static: 0, total: 50)
StringName: 5 unclaimed string names at exit.

this is on top of all the InputEventMouseMotion events which I believe to be unrelated to the initial issue.

@scgm0 scgm0 changed the title Prevent repeated instantiation of static string name Prevent repeated instantiation or be disposed of Static StringName Sep 6, 2024
@scgm0 scgm0 changed the title Prevent repeated instantiation or be disposed of Static StringName Prevent repeated instantiation or disposed of Static StringName Sep 6, 2024
@scgm0 scgm0 changed the title Prevent repeated instantiation or disposed of Static StringName Prevent repeated instantiation and release of Static StringName Sep 6, 2024
@raulsntos
Copy link
Owner

I'm not sure we want to ensure static StringNames are only instantiated once. I feel like this makes StringName more complicated and I'd prefer to keep primitive types like StringName very simple.

If we need a mechanism to avoid creating the same StringName more than once, we may want to implement such mechanism on top of StringName rather than modifying the StringName type itself, but I'm not sure we need it.

Since the motivation behind this PR is to fix #20, let's try to figure out if there's a simpler way to fix this (I've commented on that issue: #20 (comment)).

@scgm0
Copy link
Contributor Author

scgm0 commented Sep 10, 2024

I'm not sure we want to ensure static StringNames are only instantiated once. I feel like this makes StringName more complicated and I'd prefer to keep primitive types like StringName very simple.

If we need a mechanism to avoid creating the same StringName more than once, we may want to implement such mechanism on top of StringName rather than modifying the StringName type itself, but I'm not sure we need it.

Since the motivation behind this PR is to fix #20, let's try to figure out if there's a simpler way to fix this (I've commented on that issue: #20 (comment)).

I kept the static StringName instantiated only once in this pr because I did not find how to determine whether the native StringName passed by godot is a static StringName. If the native StringName is static, but the instance created by c# is not static, then the c# instance It will be recycled by gc soon and trigger the recycling of native StringName, eventually causing the static StringName reference in godot to be 0.
Is there a way for us to determine whether the native StringName is static? In this way, there will be no problem even if StringName is instantiated repeatedly in c#.

@scgm0 scgm0 force-pushed the Prevent-repeated-instantiation-of-static-StringName branch from 29ee39c to abf44e7 Compare September 10, 2024 21:28
@scgm0 scgm0 changed the title Prevent repeated instantiation and release of Static StringName Prevent release of Static StringName Sep 12, 2024
@scgm0 scgm0 changed the title Prevent release of Static StringName Prevent dispose of Static StringName Sep 12, 2024
@scgm0
Copy link
Contributor Author

scgm0 commented Sep 12, 2024

It seems to fill the need, but it's not public. . .
图片

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lots of Unreferenced static string to 0: errors when closing a scene
3 participants