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.

Sign up for GitHub

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 generic arrays and dictionaries in .NET not calling set_typed #98545

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Fix generic arrays and dictionaries in .NET not calling set_typed #98545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions modules/mono/csharp_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2344,9 +2344,7 @@ bool CSharpScript::can_instantiate() const {
}

StringName CSharpScript::get_instance_base_type() const {
StringName native_name;
GDMonoCache::managed_callbacks.ScriptManagerBridge_GetScriptNativeName(this, &native_name);
return native_name;
return type_info.native_base_name;
}

CSharpInstance *CSharpScript::_create_instance(const Variant **p_args, int p_argcount, Object *p_owner, bool p_is_ref_counted, Callable::CallError &r_error) {
Expand Down
5 changes: 5 additions & 0 deletions modules/mono/csharp_script.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ class CSharpScript : public Script {
*/
String class_name;

/**
* Name of the native class this script derives from.
*/
StringName native_base_name;

/**
* Path to the icon that will be used for this class by the editor.
*/
Expand Down
19 changes: 19 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,22 @@ private static godot_variant ToVariantFunc(in Array<T> godotArray) =>
private static Array<T> FromVariantFunc(in godot_variant variant) =>
VariantUtils.ConvertToArray<T>(variant);

private void SetTypedForUnderlyingArray()
{
Marshaling.GetTypedCollectionParameterInfo<T>(out var elemVariantType, out var elemClassName, out var elemScriptRef);

var self = (godot_array)NativeValue;

using (elemScriptRef)
{
NativeFuncs.godotsharp_array_set_typed(
ref self,
(uint)elemVariantType,
elemClassName,
elemScriptRef);
}
}

static unsafe Array()
{
VariantUtils.GenericConversion<Array<T>>.ToVariantCb = &ToVariantFunc;
Expand All @@ -1083,6 +1099,7 @@ internal ref godot_array.movable NativeValue
public Array()
{
_underlyingArray = new Array();
SetTypedForUnderlyingArray();
}

/// <summary>
Expand All @@ -1099,6 +1116,7 @@ public Array(IEnumerable<T> collection)
throw new ArgumentNullException(nameof(collection));

_underlyingArray = new Array();
SetTypedForUnderlyingArray();
Comment on lines 1118 to +1119
Copy link
Contributor

Choose a reason for hiding this comment

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

now that there is more common logic, should these array and dictionary constructors generally use : this() { .. } or is it still worth checking/throwing prior?


foreach (T element in collection)
Add(element);
Expand All @@ -1118,6 +1136,7 @@ public Array(T[] array)
throw new ArgumentNullException(nameof(array));

_underlyingArray = new Array();
SetTypedForUnderlyingArray();

foreach (T element in array)
Add(element);
Expand Down
22 changes: 8 additions & 14 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,18 +182,7 @@ internal static unsafe void GetScriptNativeName(IntPtr scriptPtr, godot_string_n
return;
}

var native = GodotObject.InternalGetClassNativeBase(scriptType);

var field = native.GetField("NativeName", BindingFlags.DeclaredOnly | BindingFlags.Static |
BindingFlags.Public | BindingFlags.NonPublic);

if (field == null)
{
*outRes = default;
return;
}

var nativeName = (StringName?)field.GetValue(null);
var nativeName = GodotObject.InternalGetClassNativeBaseName(scriptType);

if (nativeName == null)
{
Expand Down Expand Up @@ -658,10 +647,14 @@ internal static godot_bool TryReloadRegisteredScriptWithClass(IntPtr scriptPtr)

private static unsafe void GetScriptTypeInfo(Type scriptType, godot_csharp_type_info* outTypeInfo)
{
Type native = GodotObject.InternalGetClassNativeBase(scriptType);

godot_string className = Marshaling.ConvertStringToNative(ReflectionUtils.ConstructTypeName(scriptType));

StringName? nativeBase = GodotObject.InternalGetClassNativeBaseName(scriptType);

godot_string_name nativeBaseName = nativeBase != null
? NativeFuncs.godotsharp_string_name_new_copy((godot_string_name)nativeBase.NativeValue)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to copy the StringName?

Copy link
Contributor Author

@juanjp600 juanjp600 Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fairly certain that this is necessary. At the very least, it's consistent with how ScriptManagerBridge.GetScriptNativeName handles passing a managed StringName to unmanaged code.

If I'm understanding this correctly, it's effectively the same as an implicit call to StringName's copy constructor within C++. This is important to maintain a correct refcount; I tested what happens when a copy isn't made, and I get the following errors when I close the editor:

ERROR: BUG: Unreferenced static string to 0: EditorExportPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: HBoxContainer
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: EditorPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: MarginContainer
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: EditorInspectorPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: RefCounted
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: Node2D
   at: unref (core/string/string_name.cpp:144)

I suppose given the static lifetime of the StringNames returned by GodotObject.InternalGetClassNativeBaseName, it might be safe to pass a pointer to the underlying native form of that instead of constructing a new one, but I am not 100% sure about that so I went with this since I'm more confident that it's safe.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think you are 100% correct. I was also thinking about passing a pointer to the underlying native form, but I agree it's safer to do the copy.

: default;

bool isTool = scriptType.IsDefined(typeof(ToolAttribute), inherit: false);

// If the type is nested and the parent type is a tool script,
Expand All @@ -686,6 +679,7 @@ private static unsafe void GetScriptTypeInfo(Type scriptType, godot_csharp_type_
godot_string iconPath = Marshaling.ConvertStringToNative(iconAttr?.Path);

outTypeInfo->ClassName = className;
outTypeInfo->NativeBaseName = nativeBaseName;
outTypeInfo->IconPath = iconPath;
outTypeInfo->IsTool = isTool.ToGodotBool();
outTypeInfo->IsGlobalClass = isGlobalClass.ToGodotBool();
Expand Down
23 changes: 23 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,27 @@ private static godot_variant ToVariantFunc(in Dictionary<TKey, TValue> godotDict
private static Dictionary<TKey, TValue> FromVariantFunc(in godot_variant variant) =>
VariantUtils.ConvertToDictionary<TKey, TValue>(variant);

private void SetTypedForUnderlyingDictionary()
{
Marshaling.GetTypedCollectionParameterInfo<TKey>(out var keyVariantType, out var keyClassName, out var keyScriptRef);
Marshaling.GetTypedCollectionParameterInfo<TValue>(out var valueVariantType, out var valueClassName, out var valueScriptRef);

var self = (godot_dictionary)NativeValue;

using (keyScriptRef)
using (valueScriptRef)
{
NativeFuncs.godotsharp_dictionary_set_typed(
ref self,
(uint)keyVariantType,
keyClassName,
keyScriptRef,
(uint)valueVariantType,
valueClassName,
valueScriptRef);
}
}

static unsafe Dictionary()
{
VariantUtils.GenericConversion<Dictionary<TKey, TValue>>.ToVariantCb = &ToVariantFunc;
Expand All @@ -519,6 +540,7 @@ internal ref godot_dictionary.movable NativeValue
public Dictionary()
{
_underlyingDict = new Dictionary();
SetTypedForUnderlyingDictionary();
}

/// <summary>
Expand All @@ -535,6 +557,7 @@ public Dictionary(IDictionary<TKey, TValue> dictionary)
throw new ArgumentNullException(nameof(dictionary));

_underlyingDict = new Dictionary();
SetTypedForUnderlyingDictionary();

foreach (KeyValuePair<TKey, TValue> entry in dictionary)
Add(entry.Key, entry.Value);
Expand Down
54 changes: 49 additions & 5 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/GodotObject.base.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.InteropServices;
using Godot.Bridge;
using Godot.NativeInterop;
Expand All @@ -13,6 +15,8 @@ public partial class GodotObject : IDisposable
private bool _disposed;
private static readonly Type _cachedType = typeof(GodotObject);

private static readonly Dictionary<Type, StringName?> _nativeNames = new Dictionary<Type, StringName?>();

internal IntPtr NativePtr;
private bool _memoryOwn;

Expand Down Expand Up @@ -191,16 +195,56 @@ public SignalAwaiter ToSignal(GodotObject source, StringName signal)
return new SignalAwaiter(source, signal, this);
}

internal static bool IsNativeClass(Type t)
{
if (ReferenceEquals(t.Assembly, typeof(GodotObject).Assembly))
{
return true;
}

if (ReflectionUtils.IsEditorHintCached)
{
return t.Assembly.GetName().Name == "GodotSharpEditor";
}

return false;
}

internal static Type InternalGetClassNativeBase(Type t)
{
var name = t.Assembly.GetName().Name;
while (!IsNativeClass(t))
{
Debug.Assert(t.BaseType is not null, "Script types must derive from a native Godot type.");

t = t.BaseType;
}

return t;
}

internal static StringName? InternalGetClassNativeBaseName(Type t)
{
if (_nativeNames.TryGetValue(t, out var name))
{
return name;
}

var baseType = InternalGetClassNativeBase(t);

if (_nativeNames.TryGetValue(baseType, out name))
{
return name;
}

var field = baseType.GetField("NativeName",
BindingFlags.DeclaredOnly | BindingFlags.Static |
BindingFlags.Public | BindingFlags.NonPublic);

if (name == "GodotSharp" || name == "GodotSharpEditor")
return t;
name = field?.GetValue(null) as StringName;

Debug.Assert(t.BaseType is not null, "Script types must derive from a native Godot type.");
_nativeNames[baseType] = name;

return InternalGetClassNativeBase(t.BaseType);
return name;
}

// ReSharper disable once VirtualMemberNeverOverridden.Global
Expand Down
7 changes: 7 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public Godot.Variant.Type Expected
public ref struct godot_csharp_type_info
{
private godot_string _className;
private godot_string_name _nativeBaseName;
private godot_string _iconPath;
private godot_bool _isTool;
private godot_bool _isGlobalClass;
Expand All @@ -122,6 +123,12 @@ public godot_string ClassName
set => _className = value;
}

public godot_string_name NativeBaseName
{
readonly get => _nativeBaseName;
set => _nativeBaseName = value;
}

public godot_string IconPath
{
readonly get => _iconPath;
Expand Down
38 changes: 38 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/Marshaling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,44 @@ internal static Variant.Type ConvertManagedTypeToVariantType(Type type, out bool
return Variant.Type.Nil;
}

internal static void GetTypedCollectionParameterInfo<T>(
out Variant.Type variantType,
out godot_string_name className,
out godot_ref script)
{
variantType = ConvertManagedTypeToVariantType(typeof(T), out _);

if (variantType != Variant.Type.Object)
{
className = default;
script = default;
return;
}

godot_ref scriptRef = default;

if (!GodotObject.IsNativeClass(typeof(T)))
{
unsafe
{
Godot.Bridge.ScriptManagerBridge.GetOrLoadOrCreateScriptForType(typeof(T), &scriptRef);
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing we don't expect this to ever create a script here, since if T is not a native class it must be a C# script so a script reference should already exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible for the construction of a collection to trigger ScriptManagerBridge.CreateScriptBridgeForType. Here's an example project illustrating this: RecursiveReferenceThruArrays.zip

The stacktrace for such a call looks like this:

   at System.Environment.get_StackTrace()
   at Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 565
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 469
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(godot_string* scriptPath, godot_string* c_backtrace, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 452
   at Godot.Bridge.ScriptManagerBridge.GetOrLoadOrCreateScriptForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 540
   at Godot.NativeInterop.Marshaling.GetTypedCollectionParameterInfo[T](Type& variantType, godot_string_name& className, godot_ref& script) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/Marshaling.cs:line 222
   at Godot.Collections.Array`1.SetTypedForUnderlyingArray() in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs:line 1065
   at Godot.Collections.Array`1..ctor() in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs:line 1102
   at ScriptNodeA.GetGodotPropertyDefaultValues() in /home/juan/GodotMrps/RecursiveReferenceThruArrays/Godot.SourceGenerators/Godot.SourceGenerators.ScriptPropertyDefValGenerator/ScriptNodeA_ScriptPropertyDefVal.generated.cs:line 15
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
   at Godot.Bridge.ScriptManagerBridge.GetPropertyDefaultValuesForType(Type type, IntPtr scriptPtr,  addDefValFunc) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 1125
   at Godot.Bridge.ScriptManagerBridge.GetPropertyDefaultValues(IntPtr scriptPtr,  addDefValFunc) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 1100
   at Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 572
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 469
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(godot_string* scriptPath, godot_string* c_backtrace, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 452

I'm not aware of any issues that might arise in this scenario.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thank you. I don't think there should be any issues with this.

}

// Don't call GodotObject.InternalGetClassNativeBaseName here!
// godot_dictionary_set_typed and godot_array_set_typed will call CSharpScript::get_instance_base_type
// when a script is passed, because this is better for performance than using reflection to find the
// native base type.
className = default;
}
else
{
StringName? nativeBaseName = GodotObject.InternalGetClassNativeBaseName(typeof(T));
className = nativeBaseName != null ? (godot_string_name)nativeBaseName.NativeValue : default;
}

script = scriptRef;
}

// String

public static unsafe godot_string ConvertStringToNative(string? p_mono_string)
Expand Down
33 changes: 33 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,14 @@ public static partial void

public static partial void godotsharp_array_make_read_only(ref godot_array p_self);

public static partial void godotsharp_array_set_typed(
ref godot_array p_self,
uint p_elem_type,
in godot_string_name p_elem_class_name,
in godot_ref p_elem_script);

public static partial godot_bool godotsharp_array_is_typed(ref godot_array p_self);

public static partial void godotsharp_array_max(ref godot_array p_self, out godot_variant r_value);

public static partial void godotsharp_array_min(ref godot_array p_self, out godot_variant r_value);
Expand Down Expand Up @@ -463,6 +471,31 @@ public static partial godot_bool godotsharp_dictionary_remove_key(ref godot_dict

public static partial void godotsharp_dictionary_make_read_only(ref godot_dictionary p_self);

public static partial void godotsharp_dictionary_set_typed(
ref godot_dictionary p_self,
uint p_key_type,
in godot_string_name p_key_class_name,
in godot_ref p_key_script,
uint p_value_type,
in godot_string_name p_value_class_name,
in godot_ref p_value_script);

public static partial godot_bool godotsharp_dictionary_is_typed_key(ref godot_dictionary p_self);

public static partial godot_bool godotsharp_dictionary_is_typed_value(ref godot_dictionary p_self);

public static partial uint godotsharp_dictionary_get_typed_key_builtin(ref godot_dictionary p_self);

public static partial uint godotsharp_dictionary_get_typed_value_builtin(ref godot_dictionary p_self);

public static partial void godotsharp_dictionary_get_typed_key_class_name(ref godot_dictionary p_self, out godot_string_name r_dest);

public static partial void godotsharp_dictionary_get_typed_value_class_name(ref godot_dictionary p_self, out godot_string_name r_dest);

public static partial void godotsharp_dictionary_get_typed_key_script(ref godot_dictionary p_self, out godot_variant r_dest);

public static partial void godotsharp_dictionary_get_typed_value_script(ref godot_dictionary p_self, out godot_variant r_dest);

public static partial void godotsharp_dictionary_to_string(ref godot_dictionary p_self, out godot_string r_str);

// StringExtensions
Expand Down
8 changes: 4 additions & 4 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/ReflectionUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ internal class ReflectionUtils
{
private static readonly HashSet<Type>? _tupleTypeSet;
private static readonly Dictionary<Type, string>? _builtinTypeNameDictionary;
private static readonly bool _isEditorHintCached;
internal static readonly bool IsEditorHintCached;

static ReflectionUtils()
{
_isEditorHintCached = Engine.IsEditorHint();
if (!_isEditorHintCached)
IsEditorHintCached = Engine.IsEditorHint();
if (!IsEditorHintCached)
{
return;
}
Expand Down Expand Up @@ -66,7 +66,7 @@ static ReflectionUtils()

public static string ConstructTypeName(Type type)
{
if (!_isEditorHintCached)
if (!IsEditorHintCached)
{
return type.Name;
}
Expand Down
Loading