-
-
Notifications
You must be signed in to change notification settings - Fork 96
C# Wrapper #512
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
base: main
Are you sure you want to change the base?
C# Wrapper #512
Conversation
- Begin effort on PCam3D and some basic Godot extensions and PCam types
- added Engine singleton registration to PhantomCameraManager - fixed incorrect LookAtMode signal (not available in 2d) - added c# project files - created basic test runner and added some tests (wip)
- fix several bugs from invalid getter/setters, properties, and types - added more tests - added LimitTarget query result type for working with TileMaps and CollisionShape2Ds - reorganized scripts since main script file had become quite large
- added missing shared camera properties - added missing phantom camera 2d properties - added missing phantom camera 3d properties
- Begin effort on PCam3D and some basic Godot extensions and PCam types
Unfortunately the merge didn't go as I planned, so a LOT of the UIDs changed (just look at the files changed count, it went from 25 to 61), I don't know if that might be a problem though. I also did some changes like adding PhantomCameraNoise3D and PhantomCameraNoiseEmitter3D, more coming soon. |
I hate merging manually
🙌 this is awesome to see, thank you so much for picking this up @GabCoolDude |
@GabCoolDude I don't have a good answer for this. I think for safety it would be appropriate to offer whatever is supported by the godot objects so the interfaces match closely. One small C# specific improvement you could probably add is just having one AppendFollowTargets() method that takes a variadic number of targets. I say just follow your heart, but @ramokz may have a different opinion lol |
Yes, all public functions should be included. So if it exists in the GDScript version / documentation site, the C# wrapper should also include them. Otherwise, we end up with method availability discrepancies between GDScript and C# users. A minor note in the C# snippet above, the "s" in "Targets" is important.
Are you thinking of combining |
As an aside, think the Unit test bit should be pulled out of this PR and into a separate one. Ideally, want to keep PRs as specific as possible, so it doesn't take longer than necessary to get merged and split the review process into smaller, more specific, chunks. |
I agree, especially since I am having a bit of trouble with it currently. Will be removed in the next commit. |
Added missing setters: if a variable had multiple setters, they were missing, so they were added.
Why do we use constructors for the classes, instead of inheriting something like var pcam3D = GetNode<Node3d>("path/to/Node3D").AsPhantomCamera3d(); We could be doing var pcam3D = GetNode<PhantomCamera3D>("path/to/Node3D"); which is the default way to use type casts in C# with Godot. Currently, we are using a method that would require extra documentation and is different than every other way to type cast with a This is also probably the reason that Dialogue Manager's wrapper inherits RefCounted, because RefCounted is a type used for many resources, and also because it inherits |
I have been wondering about that as well. I never dug deep enough into C# wrappers to know whether if there's a technical limitation for the current approach, but think it's definitely worth exploring your suggestion. |
Should I put doc comments on each variable and class, just like in GDScript, or should I leave them be ? I think they'd be quite useful, because you wouldn't have to pull up the documentation in a browser tab and would just hover over the property to see what it does. |
From a DX perspective, it makes total sense. However, from a maintainability perspective, I am reluctant to add it simply because of the extra work to keep descriptions in sync across the various places. Think it's one of those things where once you start doing it, you have to keep doing it. You can also access the documentation from Godot directly by looking it up by using |
This reverts commit 902b2f1.
Turns out that the method we had before was the best way to go about it without creating C# bindings using C++. I had a little conversation in the Godot Engine discord related to an error I had, which would've caused the whole C# wrapper to not work with |
Sorry I wasn't able to provide this insight sooner. I saw your question about this last night and I could not recall the exact reason I chose that method. I thought that it was a DX choice because it also allowed the ability to create the C# extension methods. But I think I remember having issues trying the Some of the choices I made were also because I was looking at other popular plugins to see their approach to adding C# support. I think DialogueManager was one I took a lot of inspiration from. Now that you've spent some time with the code, it might be worth looking through the conversations on the old PR again to see if there are any useful insights there. Also, nice work on this! |
That's a good discovery! Thanks for looking into it. Yeah, let's go with what the previous iteration was doing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a high-level review of the PR, have yet to dig into the various class files.
Still trying to get a custom build of Godot mono working to test various things, but it is proving more cumbersome than I had hoped.
addons/phantom_camera/examples/textures/3D/checker_pattern_dark.png.import
Outdated
Show resolved
Hide resolved
addons/phantom_camera/examples/textures/3D/checker_pattern_dark.png.import
Outdated
Show resolved
Hide resolved
- Removed dedicated dev scene for C# (and its script) - Removed junk files from the .csproj file - Fixed dev scene 3D having the wrong root node
You can just download the .NET version from the website. If you use the Steam version of Godot, you can still use it with this method. |
Thanks! But was referring to compiling a mono build of the Godot engine itself. It might just be more complicated due to my OS environment not playing nicely with mono / dotnet. At the minute, the custom build thinks I'm using an outdated C# version, which doesn't really make sense… It works fine with a typical Godot mono executable, so it's likely a build process step on my part. For a bit of context, the reason I need to compile the engine locally is to test a few custom export templates that most users will not need, but those who do will raise an issue about it. The reason it matters is that it might mean the way the C# classes are currently set up would need a structural change. |
- Removed ReSharper comment in PhantomCamera2D.cs - Added getters in PhantomCameraManager.cs - Removed the android and ios specific parts of the csproj
(idk why it was changed, it wasnt me, i just tried to fix it)
Using GDScript with c# is extremely annoying, so having a C# wrapper will make things much easier. Closes #286. Based on #351.
Changes
All underlying object types can be reference by calling the node type field:
Each class also follows the Godot convention of having a
MethodName
field with all the string constants for the gdscript getters and setters. PhantomCamera contains the shared strings, PhantomCamera2D and PhantomCamera3D contain their own unique strings.