MatejSloboda / Dijkstra_map_for_Godot

MIT License
78 stars 13 forks source link

Docs for C# #124

Closed SimonasLetukas closed 1 year ago

SimonasLetukas commented 1 year ago

Hi

After I got the code working using C# I thought I'd ask whether you guys think it would be a good idea to add some C# guidance and snippets to the existing documentation?

I can definitely do it, no problem, just wanted to know your opinion first :)

On top of that, maybe adding a C# interface to the package would also be a good idea? So that in C# code users could just do

_pathfinding = new DijkstraMap();
_pathfinding.AddSquareGrid(...);

and not write their own interfaces that call the the non-typed GDNative methods behind the scenes

astrale-sharp commented 1 year ago

Hey there! thanks for your interest!

I'm personally not against C# docs or binding but I'm not entirely sure if we should add an interface (I'd be curious to know what @arnaudgolfouse thinks about it.

I feel like you call it untyped from gdscript and everything is fine but I never did C#!

If you're interested in providing a small working testable example, It might help me see the interest of your proposal!

SimonasLetukas commented 1 year ago

Hi Astrale,

It's curious that you say you never did C# even though your name has "sharp" in it haha :D

Anyway, I have opened a quick PR with a working example of what I mean: https://github.com/MatejSloboda/Dijkstra_map_for_Godot/pull/125

My idea was that the public class DijkstraMap in my example could be bundled together with the library so that C# users could use it out of the box so that they don't have to do NativeScript instantiation as well as the string-based GDNative calls themselves.

In other words, right now in GDScript you can do

dijkstramap = DijkstraMap.new()
dijkstramap.add_square_grid(bmp)

but you can't do this in C#

_dijkstramap = new DijkstraMap();
_dijkstramap.AddSquareGrid(bmp);

so my proposal is to move the C# experience closer to that of GDScript by introducing a DijkstraMap interface / wrapper.

Now in terms of logistics, I can write and test such wrapper and open a PR, but I need help from you to tell me where to put such wrapper class in the project structure & also if it's OK to change the project to mono etc

astrale-sharp commented 1 year ago

I have all the sharp i need already :sunglasses:

It's no big deal to "change" the repo to mono: we don't bundle godot here so it already works weither our users have mono or not. Adding some cs files also don't mean our user must have a mono version so entirely not against this!

the files could be put in addons/dijkstra-map/c-sharp-integration or something so that they get downloaded via the asset store (on a new release from us)

This new directory should contain a easy to test for users (that downlaoded only the addons/dijkstra-map folder) c# impl of the vizualisation demo

The read me should also be updated accordingly

Then I'll test a bit, tell you what I think, change my name to astrale-script and merge probably ;)

SimonasLetukas commented 1 year ago

Hi astrale-rust,

Thanks for the reply! Since the gdscript's DijkstraMap is accessed from addons/dijkstra-map/Dijkstra_map_library/nativescript.gdns I'm considering adding the DijkstraMap.cs there.

The visualization demo is already updated in my PR and gives a good intro I hope. Only need to update the docs to make sure that the switch to C# in the scene is made obvious.

I'll also have a look if I can add "integration" tests to make sure that all of the wrapper interfaces work as expected.

My final concern is the discoverability of DijkstraMap.cs, I'm not sure what's the best way to make sure that a mono-based Godot project would detect this class -- and if it's a manual process, it should be documented.

Changed the PR to WIP, I'll post here once the PR is ready :)

astrale-sharp commented 1 year ago

I checkout out your PR and i get failed to build project solution and I'm not sure how to proceed, also gdscript should remain mostly unchanged right? since it's c sharp that's doing the work

SimonasLetukas commented 1 year ago

Hi, have you got .NET SDK and mono-enabled Godot installed? https://docs.godotengine.org/en/3.5/tutorials/scripting/c_sharp/c_sharp_basics.html

GDScript can easily co-exist with C#, that's why I added C# code for the duplicated tileset scene only, so the whole visualization demo doesn't need to be duplicated.

astrale-sharp commented 1 year ago

I didn't, I do now and I'll try again later ;)

SimonasLetukas commented 1 year ago

Alright I've updated the PR, let me know what you think.

I still need to go through all methods and test them and also figure out how to write automated tests -- they have to be initiated from Godot but written in C#. I assume you don't have any examples of this working somewhere? :D

astrale-sharp commented 1 year ago

We have an example right here, all our gdtests are autogenerated :sunglasses: , we use Gut!

I'll take a look as soon as I can,

SimonasLetukas commented 1 year ago

Unfortunately couldn't get the C# tests to work with Gut. xUnit (.NET testing framework) also doesn't seem to be working. So not really sure if we can merge the PR at all without tests... :(

astrale-sharp commented 1 year ago

I don't have much time for this these days but when I do I'll take a look at testing for C#

SimonasLetukas commented 1 year ago

Closing this because of https://github.com/MatejSloboda/Dijkstra_map_for_Godot/pull/125