Added CSharpAll variant of SimpleTestEnv

[edbeeching]

Thanks for the PR! Can you add some details about what feature/example this implements.

[Ivan-267]

@LorinczAdrien I've made a quick test and it seems to work, I only ran into an error with running the onnx inference, but the provided solution in the review fixed it for me. Nice work! I think we can merge it to my branch after this fix (you can also test if onnx inference works for you as well after applying it).

@edbeeching It is a part of our test project for working with C# envs and Godot RL (discussed on GDRL Discord).

Overview of the project:

While the ideal solution to interface C# games is a complete C# based plugin, it's not practical to write right now as it would increase the work needed for updates (can still be considered at some future point).

Instead, we tested two simpler approaches: 1) @LorinczAdrien wrote a C# wrapper of the AIController, which can be inherited from in C#, the entire game is written in C#, 2) I wrote a version that interfaces a gdscript extended AIController with the rest of the game written in C#.

The idea is to add these two approaches to the SimpleTestEnv, so users have 3 versions of the env to compare in total (GDScript only, 1, and 2 from above). These are in separate sub-folders to avoid any conflicts and/or confusion with having same named gdscript and C# classes in the same project.

The final part of the project will involve writing a documentation entry on the Godot RL Agents repository, explaining the advantages and disadvantages of each of these approaches, including links to these examples.

We're not adding files from the C# projects to the plugin repository for now (instead, they will be explained in the docs), as the approach might need updating with future plugin changes/updates, so it's more of a starting point for users that want to use C# with GDRL.

[LorinczAdrien]

Thanks for the review!

Regarding code review

Could you please specify which error you had when onnx_model was a Node instead of a Resource? I've tested with both, and they work in my project (I'm using Godot 4.3-dev4), I see that you were using Godot 4.3-dev5, but that most likely should not matter. I will change it, I'm just curious.

Regarding C# wrapper and documentation

I was skeptical as well about the C# wrapper, but it seems this may be a quite comfortable solution for all C# users. We discussed the problem of most likely having to port sensors, but as we've seen in this version it seems quite easy to use, I haven't used other sensors, but from what I can guess you're most likely just querying one or two things which should be fine. It still stands that this should not be present in the addon source code, but I think it may be a more preferable solution than 2) for some users (personally I prefer it too), all of this to say that I think it's worth exploring both of these approaches in detail in the documentation.

What about 2D?

Currently, this example is 3D, but since the two ai_controller_(2d/3d).gd implementations are basically the same so we could mention just changing the AIControllerSharp3D's name, and it should inherit from Node2D and _player var type to Node2D.

Maybe for completeness’s sake, we could offer a 2D example as well? Just throwing around ideas, I'd be happy to make those as well.

Wrapper of C# wrapper

Yes. In my project, I'm actually using a wrapper over this to follow C# naming conventions closely, since it's a little strange using GDScript name convention when calling methods/accessing attributes. I will link my version here to get an idea (it has some random comments, that may be incorrect and some exports missing), of what it would mean. It does introduce another layer of responsibility to the user, that if anything should change two C# files should be changed, but we'll see what you think.

[LorinczAdrien]

Another thing @Ivan-267, I've seen that in the environments you set up the player is made to have 'Editable Children', from what I've seen you are not using any of the children references or rewriting anything in them (that would be usually the use-case for this option). Is there any reason for using this option, or can we revert this to make the example a little cleaner?

[Ivan-267]

Could you please specify which error you had when onnx_model was a Node instead of a Resource? I've tested with both, and they work in my project (I'm using Godot 4.3-dev4), I see that you were using Godot 4.3-dev5, but that most likely should not matter. I will change it, I'm just curious.

The reason is that GDScript class ONNXModel extends Resource rather than Node, so since we can't use the GDScript classname in C#, we can still use the Godot class it inherits from.

https://github.com/edbeeching/godot_rl_agents_examples/blob/31f30ac5405887c54c679be597c695fecdc092d8/examples/TestExamples/SimpleReachGoal/CSharpAll/addons/godot_rl_agents/onnx/wrapper/ONNX_wrapper.gd#L1

If I do set it to Node and try running the scene in Onnx Inference mode, the error that I get is:

If this doesn't happen in previous Godot versions, maybe they added an extra check which helped us to detect this in this case. You did run into some (different) errors related to ONNXModel with your C# project, I wonder if this might also help with that.

What about 2D?

This is a good question, I think for these PRs and this example, we probably don't need a 2D variant since it couldn't be used in the example. Also, while it is a very small change, it increases the "getting outdated" issue a bit when we provide both.

So, we have two possible options here (not necessarily exclusive): 1 - Mention to the users how to make the small change for the 2D case in the documentation guide 2 - Make a future 2D example for it

Personally I'm not sure if we need a special 2D example just for this project, and the more examples we make with C#, the more we increase the "getting outdated" issue (unfortunately).

We can consider it of course (having more examples is good). It might be possible to have examples that are set to work with specific versions of GDRL/Plugin (then they can't get outdated if they don't use any newer features - as long as those versions GDRL/Plugin versions are still available and functioning), as long as we can explain it clearly and it's not too complex to run the example.

I already had an example that uses a discrete GDRL branch (which is now a bit outdated feature-wise), the 3DLander example, although it might be update-able to Rllib with the latest branch which supports discrete actions, and/or I might be able to re-implement support for SB3 discrete actions on top of the latest changes in a simpler manner.

Wrapper of C# wrapper

Of course this looks good. In your opinion, could all of this go into the initial wrapper (so that only the CSharp-style named methods need to be extended), so we don't have to have a wrapper over wrapper, or would that increase the complexity of the wrapper too much? Especially since users might need to modify the wrapper if they wish to add some new features in the future, I wonder if a single wrapper might be more simple from that sense (mod-ability), however, we can consider both options, or even the current option.

[Ivan-267]

Another thing @Ivan-267, I've seen that in the environments you set up the player is made to have 'Editable Children', from what I've seen you are not using any of the children references or rewriting anything in them (that would be usually the use-case for this option). Is there any reason for using this option, or can we revert this to make the example a little cleaner?

Nicely spotted. It's possible I had something connected in a different structure during development, I think it might not be necessary now as long as we connect the AIController in the Player scene (which would be the proper way).

You can try it, briefly retest onnx inference and training (just starting training should be sufficient), and if it works (I think it should work unless I forgot something), feel free to update it in this branch. I won't change the original PR to keep things simple for merging your changes. You can modify all 3 env versions if you like in this PR, or I can modify the other 2 after we merge your PR, whichever method you prefer.

[LorinczAdrien]

Changes

I've fixed the onnx_model in the new commit and updated all 3 envs by removing the 'Editable Children' option from the player node.

Other changes are still being discussed.

[LorinczAdrien]

After further discussions, we decided to stay with the simplest approach to the C# wrapper, since it provides the most flexibility and it should be easier to understand without the wrapper. We will not make a 2D example (atleast not now), but we will document the process of rewriting the C# 3D AIController into a 2D one. Otherwise, the last commit has all the required changes to merge this pull request.

[Ivan-267]

@LorinczAdrien As far as I'm concerned, you can feel free to merge this.

When we have the docs done, we can do a final review and make any changes if needed in AddSimpleTestEnv before merging everything into main.