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

Godot ignores namespace information when instantiating C# classes #17409

Closed
mysticfall opened this issue Mar 10, 2018 · 5 comments · Fixed by #23162
Closed

Godot ignores namespace information when instantiating C# classes #17409

mysticfall opened this issue Mar 10, 2018 · 5 comments · Fixed by #23162

Comments

@mysticfall
Copy link
Contributor

mysticfall commented Mar 10, 2018

Godot version:
master / dad47d8

OS/device including version:
Manjaro 17.1

Issue description:
Godot apparently doesn't take the namespace information into account when instantiating a class that is assigned to a node.

Steps to reproduce:

  • Create Namespace2/MyClass.cs and assign it to a node.
  • Override Namespace2.MyClass._Ready() and add GD.Print("MyClass 2").
  • Run the project and verify MyClass 2 is printed in the console.
  • Create Namespace1/MyClass.cs but do not assign it to anything.
  • Override Namespace1.MyClass._Ready() and add GD.Print("MyClass 1").
  • Run the project again, and verify MyClass 1 is printed instead of MyClass 2.
@mysticfall
Copy link
Contributor Author

It's a prerequisite to #15661, and probably also to #15237.

In #15237, I thought it just doesn't handle namespace informations correctly just for addon projects, but it seems like Godot relies soley on a class name derived from a source path, ignoring both the assembly and namespace information for all cases.

@reduz
Copy link
Member

reduz commented Mar 10, 2018

From what I discussed with @neikeq , obtaining the actual class from the file with a tag should be easy. Either using roslyn of with a hand made parser (tokenizing C# is super easy)

@mysticfall
Copy link
Contributor Author

mysticfall commented Mar 10, 2018

I agree that probably parsing the source file could be the easiest way to solve this problem for now. (It wouldn't support declaring more than one classes in a single source file, but maybe it's a corner case.)

@reduz I'm sorry I've dragged you around on many different issues. I initially thought this should have a more impact on the original issue, but now I realized the worst scenario wouldn't be more than simply abandoning the suggested parser code, which probably is not much of a loss.

So, I think it'd be better to fix this issue in a way that can be done with the minimal efforts, and continue to discuss if all related cases like #15237 can be supported using the current source based approach later.

@neikeq
Copy link
Contributor

neikeq commented Mar 10, 2018

Yes, this is a known limitation that will be fixed soon. I was waiting to finish the exporter in order to implement this. The way Godot will detect the qualified name for the script is the following:

  • A simple parser to find the class and its namespace that share the same name as its file. We can switch to Microsoft.CodeAnalysis.CSharp if our own parser proves hard to maintain. The subset of C# we need to parse is very small though and Microsoft.CodeAnalysis.CSharp would mean +9MB size for the editor
  • [GodotScript] attribute. This attribute will use [CallerFilePath] to get the file path. Here is an example: https://gist.github.com/neikeq/db685eda63e558d207abf12dececa51b.
    The purpose of this attribute is to allow, in the future, other languages (see Explicit support for non-C# CLR languages #12337) to support namespaces without needing a parser.
    I'm not sure if this attribute should have priority over the class found by the parser. If it does, that would mean you could specify a different class in a file to represent the script, instead of the one with the file name. This could be useful, but also confusing.
    The problem with [CallerFilePath] is that it uses absolute paths, which is probably undesired, so we would either have to strip it from the assembly with Cecil or surround it with #if TOOLS, so its not included in exported games (the relative paths would be stored in a metadata file at export time).
  • If the class could not be found with neither of the previous methods, we either show an error or fallback to the current method we have (iterate the classes in an assembly, and find the first one with the file name).

@exts
Copy link
Contributor

exts commented Jul 11, 2018

@neikeq what's the status of this issue? Just tested and it looks like it grabs the last class w/ the same name in the item group and executes it.

  <ItemGroup>
    <Compile Include="Namespace1\MyClass.cs" />
    <Compile Include="Namespace2\MyClass.cs" />
    <Compile Include="Namespace3\MyClass.cs" />
    <Compile Include="Properties\AssemblyInfo.cs" />
  </ItemGroup>

This could be a huge issue with larger projects and could put off a lot of people wanting to use the c# godot version, as this is a show stopper.

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

Successfully merging a pull request may close this issue.

4 participants