Open Yam-s opened 3 years ago
I'm looking into this now out of interest, and I'm pretty sure it's because the mono glue generates a file called System.cs
which maps Godex's System class. This then overrides the native C# System
type, which means when other classes in the generated glue reference e.g. System.Array
, i.e. the built-in C# array type, it's instead looking for it in the System.cs
file.
I haven't yet got the whole thing compiled and working, but you can fix the syntax errors in the glue manually (there are only a few) by using aliased imports for the relevant types, e.g. in modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs
you can add:
using SystemArray = System.Array;
To the using directives, and then replace instances of System.Array
with SystemArray
.
However, I'm expecting this might cause further effects when trying to write C# scripts in Godot, so it may be better to somehow change how the glue is generated for the Godex classes so that the types don't clash.
Alternatively I suspect if you build the initial binary without the Godex module, then generate the glue, then compile the final binary with Godex, it should build successfully (haven't tried it, but I'm 99% sure). Of course, you wouldn't then have a native mapping for Godex in C# which I guess is kinda the point of building it with Mono, though you could build a facade or adaptor or something manually.
Going to continue playing with it and see what comes up.
Btw, here is a git patch that you can apply to the generated glue to fix the syntax, so it should at least build.
Index: modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs (revision a3850ba05094ce6026c6a60241e1bd82f1d3b826)
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs (date 1637237399009)
@@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
+using SystemEnvironment = System.Environment;
namespace Godot
{
@@ -243,7 +244,7 @@
/// </summary>
public static void PrintStack()
{
- Print(System.Environment.StackTrace);
+ Print(SystemEnvironment.StackTrace);
}
/// <summary>
Index: modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs (revision a3850ba05094ce6026c6a60241e1bd82f1d3b826)
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs (date 1637237368889)
@@ -4,6 +4,7 @@
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Diagnostics.CodeAnalysis;
+using SystemArray = System.Array;
namespace Godot.Collections
{
@@ -202,7 +203,7 @@
/// </summary>
/// <param name="array">The array to copy to.</param>
/// <param name="index">The index to start at.</param>
- public void CopyTo(System.Array array, int index)
+ public void CopyTo(SystemArray array, int index)
{
if (array == null)
throw new ArgumentNullException(nameof(array), "Value cannot be null.");
Index: modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs (revision a3850ba05094ce6026c6a60241e1bd82f1d3b826)
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs (date 1637237345131)
@@ -3,6 +3,7 @@
using System.Collections;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
+using SystemArray = System.Array;
namespace Godot.Collections
{
@@ -29,7 +30,7 @@
/// Wrapper around Godot's Array class, an array of Variant
/// typed elements allocated in the engine in C++. Useful when
/// interfacing with the engine. Otherwise prefer .NET collections
- /// such as <see cref="System.Array"/> or <see cref="List{T}"/>.
+ /// such as <see cref="SystemArray"/> or <see cref="List{T}"/>.
/// </summary>
public class Array : IList, IDisposable
{
@@ -234,7 +235,7 @@
/// </summary>
/// <param name="array">The array to copy to.</param>
/// <param name="index">The index to start at.</param>
- public void CopyTo(System.Array array, int index)
+ public void CopyTo(SystemArray array, int index)
{
if (array == null)
throw new ArgumentNullException(nameof(array), "Value cannot be null.");
@@ -275,7 +276,7 @@
internal static extern IntPtr godot_icall_Array_Ctor();
[MethodImpl(MethodImplOptions.InternalCall)]
- internal static extern IntPtr godot_icall_Array_Ctor_MonoArray(System.Array array);
+ internal static extern IntPtr godot_icall_Array_Ctor_MonoArray(SystemArray array);
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern void godot_icall_Array_Dtor(IntPtr ptr);
@@ -305,7 +306,7 @@
internal static extern bool godot_icall_Array_Contains(IntPtr ptr, object item);
[MethodImpl(MethodImplOptions.InternalCall)]
- internal static extern void godot_icall_Array_CopyTo(IntPtr ptr, System.Array array, int arrayIndex);
+ internal static extern void godot_icall_Array_CopyTo(IntPtr ptr, SystemArray array, int arrayIndex);
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern IntPtr godot_icall_Array_Duplicate(IntPtr ptr, bool deep);
Index: modules/mono/glue/GodotSharp/GodotSharp/Core/Attributes/AssemblyHasScriptsAttribute.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Attributes/AssemblyHasScriptsAttribute.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Attributes/AssemblyHasScriptsAttribute.cs
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Attributes/AssemblyHasScriptsAttribute.cs (revision a3850ba05094ce6026c6a60241e1bd82f1d3b826)
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Attributes/AssemblyHasScriptsAttribute.cs (date 1637237626064)
@@ -1,4 +1,5 @@
using System;
+using SystemType = System.Type;
namespace Godot
{
@@ -6,14 +7,14 @@
public class AssemblyHasScriptsAttribute : Attribute
{
private readonly bool requiresLookup;
- private readonly System.Type[] scriptTypes;
+ private readonly SystemType[] scriptTypes;
public AssemblyHasScriptsAttribute()
{
requiresLookup = true;
}
- public AssemblyHasScriptsAttribute(System.Type[] scriptTypes)
+ public AssemblyHasScriptsAttribute(SystemType[] scriptTypes)
{
requiresLookup = false;
this.scriptTypes = scriptTypes;
Note: We chatted about this in discord and we contacted the Godot team to have support for that, here the discord chat link: https://discord.com/channels/797456114404163625/797456114404163630/852560754611322930
Note: We chatted about this in discord and we contacted the Godot team to have support for that, here the discord chat link: https://discord.com/channels/797456114404163625/797456114404163630/852560754611322930
Great!
FWIW, Godot 4 is in the process of moving off of mono, onto dotnet6. It still uses a similar "generated glue" approach, but it could be different enough/more modern such that it avoids this issue. I've been testing out the dotnet6 branch recently but haven't tried to build godex on it. I've no idea if it'll make a difference, but I'll give it a go when I can.
This is great!! Thx for the update, really appreciated 🙂🤙
On Thu, 28 Apr 2022 at 21:20, Lewis James @.***> wrote:
Note: We chatted about this in discord and we contacted the Godot team to have support for that, here the discord chat link: https://discord.com/channels/797456114404163625/797456114404163630/852560754611322930
Great!
FWIW, Godot 4 is in the process of moving off of mono, onto dotnet6. It still uses a similar "generated glue" approach, but it could be different enough/more modern such that it avoids this issue. I've been testing out the dotnet6 branch recently but haven't tried to build godex on it. I've no idea if it'll make a difference, but I'll give it a go when I can.
— Reply to this email directly, view it on GitHub https://github.com/GodotECS/godex/issues/187#issuecomment-1112571855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7UYRZZQOCFOEJQ2VAUVCDVHLQIVANCNFSM46OOZS4A . You are receiving this because you commented.Message ID: @.***>
Since System
is such a common namespace in C#, I would prefer to generate the class with a different name (e.g.: GodexSystem
) to avoid ambiguity errors. But that will require some other changes, as there is code that assumes C# classes don't have a different name.
I've 0 experience with C# but it seems that it's possible to use the C# namespace to fix it. In case case: GdSystem
or godex::System
or GodexSystem
seems good enough, especially if it's not required to rename the C++ class, that would affect other scripted languages.
What's the correct steps to fix this issue @neikeq?
We definitely don't want to generate a class named System
, because it will result in many ambiguity errors. It would be like calling your C++ class std
, except C# doesn't use ::
for resolving namespaces, so the compiler can't know if you want the class or the namespace. Placing that class in a different namespace, may not result in ambiguity errors when compiling the bindings, but it will result in that for whoever is using/importing that namespace.
One option is for the bindings' generator to just rename System
to System_
, or SystemClass
, or something like that, to remove ambiguity. This is the easiest solution, and it could be implemented right away today.
The other option is to generate a class named GodexSystem
. I don't suppose you want to change the name of the C++ class, so it will require some special handling by the bindings' generator. Either we add some kind of metadata information in ClassDB that informs that a class can go into the specified namespace, our we hard-code the C# bindings' generator to rename System
to GodexSystem
. The latter assumes the class comes from Godex, which may not be the case if there's another ECS module in the future.
@lewiji @Yam-s @neikeq I don't have too strong opinion on that, what do you think about System_
?
@AndreaCatania I think that's fine, since C# users will be using an ide that'll autocomplete anyway, so it's unlikely to cause confusion.
Though the cleanest solution imo would be to prefix any Godex generated classes with Godex, to prevent any other clashes/potential future clashes of namespace. Some C++ libraries use this kind of system already in godot, for example the libicu library allows a flag to add a custom prefix to statically linked functions to prevent clashes, which was recently used to prevent a clash with the dotnet bundled icu. But I'm uncertain how simple it'd be for a module to implement it on their "side" of the build, rather than having to patch the C# bindings generator itself. Classdb metadata would be best in that case as @neikeq said, but I guess would take some work to get setup to have the bindings gen detect it and output the prefixed names.
But overall I agree that simply having the generator rename with some kind of generic suffix/prefix is the simplest way and prevents any need for specific custom handling.
@lewiji Ok I was a bit worried about that thnx! So let's pick the System_
solution as it requires less work, if turns out to be an issue we can change it later.
@neikeq What's the correct way to allow that in godot?
I'll look into this next Saturday after I'm done with other tasks, and I need to make sure not to break code that assumes class names.
This diff is a working update where I changed System
to BaseSystem
https://discordapp.com/channels/797456114404163625/865464299681349662/969402587340226570
@neikeq Nice, thx a lot!
@FieldStudios If you managed to get it working already, you may submit a PR directly if you have some free time :raised_hands:
I tried to write the "System_" solution by changing https://github.com/godotengine/godot/blob/master/modules/mono/editor/bindings_generator.h#L507 to
if (itype.name == "System") {
itype.proxy_name = "System_";
} else {
itype.proxy_name = itype.name.begins_with("_") ? itype.name.substr(1, itype.name.length()) : itype.name;
}
This does generally work, but there are 2 problems:
This should be fixed in Godot 4.0+. Classes named System
are renamed to System_
.
You are right, it is.
Does Godex work with 4.x ?
From looking at the Setup and the Compiling with Mono Godot docs page, I ran the following commands to hopefully enable C# support along with Godex:
scons -j8 p=windows target=release_debug tools=yes module_mono_enabled=yes mono_glue=no custom_modules="../godex" mono_prefix="C:\Program Files\Mono"
Glue generation:
.\bin\godot.windows.opt.tools.64.mono.exe --generate-mono-glue .\modules\mono\glue
I receive the following errors when attempting to build with the glue:
scons -j8 p=windows target=release_debug tools=yes module_mono_enabled=yes mono_glue=yes custom_modules="../godex" mono_prefix="C:\Program Files\Mono"