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

feat: [Issue20] Add WeakReference to store reference types in a default provider #21

Conversation

CoreyAlexander
Copy link
Contributor

@CoreyAlexander CoreyAlexander commented Dec 17, 2024

Context

#20

When storing a reference type variable as a dependency value created in the DefaultProvider, it was causing the assemblies to fail to unload within the Godot Editor every time a build was re-built. This only appears to happen in the case where a DefaultProvider is used, likely because a strong reference is created within the AutoInject framework instead of within the Godot game using the framework.

Within the AutoInject framework, this reference should be a WeakReference instead of a strong reference to allow the Godot game to manage the reference correctly and garbage collect it when appropriate.

Considerations

  1. Does adding the weak reference create a performance issue?
  2. Does storing value types in an object type create a performance issue?

For these considerations, the best I understand is, previously storing value types within a dynamic causes them to be boxed and unboxed, which is the same as when storing them in an object type.
As well, the WeakReference acts like a boxing and unboxing as well. I believe this means that there is no performance changes between this solution and the previous one.

Testing
To test this, create a simple class with a dependency on a Godot.Resource type with the previous version of AutoInject.
Example:

[Meta(typeof(IDependent)), GlobalClass, Tool]
public partial class DependentNode: Node {
    #region Dependency Injection
    [Dependency] private Resource someResource => this.DependOn(
        () => new Resource()
    );
    #endregion
}

Following these steps you should be able to see the error:

  1. Create the above node within the scene tree.
  2. Make a change to the code of the above node.
  3. This forces you to rebuild the project.
  4. When rebuilding, you should encounter the following error and the project will need a reload to work again:
modules/mono/mono_gd/gd_mono.cpp:522 - .NET: Failed to unload assemblies. Please check https://github.com/godotengine/godot/issues/78513 for more information. (User)
  modules/mono/csharp_script.cpp:676 - Condition "managed_callable->delegate_handle.value == nullptr" is true. Continuing.

Following the same steps, with this branch, you should see no error.

@CoreyAlexander CoreyAlexander force-pushed the 20-default-providers-create-strong-references branch 4 times, most recently from ffbe1e1 to 312b9de Compare December 21, 2024 00:54
Copy link
Member

@jolexxa jolexxa left a comment

Choose a reason for hiding this comment

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

Put a few comments — only reason I call out potentially "impossible" cases (unless you deliberately circumvent null safety) is because it introduces more conditional branches which requires more tests. I haven't run this branch locally but you can check coverage with the coverage.sh script in the tests project.

@CoreyAlexander CoreyAlexander force-pushed the 20-default-providers-create-strong-references branch from 312b9de to 33a5f57 Compare December 21, 2024 14:43
@CoreyAlexander
Copy link
Contributor Author

Put a few comments — only reason I call out potentially "impossible" cases (unless you deliberately circumvent null safety) is because it introduces more conditional branches which requires more tests. I haven't run this branch locally but you can check coverage with the coverage.sh script in the tests project.

I'll have to check on the coverage. The only one that was intentional was the "Fallbacks cannot produce a null value".

@jolexxa jolexxa merged commit 7b877fa into chickensoft-games:main Dec 21, 2024
2 checks passed
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.

2 participants