WIP: Add CSharp wrapper

[sircodemane]

Context

Using C# with GD Scripts is hard, having a CSharp wrapper will make things much easier for users. Addresses #286

Changes

• Adds Godot type extensions for easy conversion:
  • Node2D.AsPhantomCamera2D()
  • Node3D.AsPhantomCamera3D()
• Adds enums used by PhantomCamera:
  • FollowMode
  • LookAtMode
  • InteractiveUpdateMode
  • TransitionType
  • EaseType
  • ProjectionType
• Adds PhantomCamera classes
  • PhantomCameraTween (Resource type)
  • Camera3DResource (Resource type)
  • PhantomCamera (abstract base class for 2d + 3d)
  • PhantomCamera2D (Node2D type)
  • PhantomCamera3D (Node3D type)
  • PhantomCameraHost (Node type)
  • PhantomCameraManager (static class)
  • ActivePhantomCameraQueryResult
    • This is a helper class used when calling PhantomCameraManager.GetActivePhantomCamera() which may be null, 2d, or 3d
  • LimitTargetQueryResult
    • This is a helper when calling PhantomCamera2D.GetLimitTarget() which may be null, TileMap, or CollisionShape2D

All underlying object types can be reference by calling the node type field:

var pcam = GetNode<Node3d>("path/to/Node3D").AsPhantomCamera3d();
pcam.Node3D.GlobalPosition = new Vector3(0, 0, 0); // access the underlying Node3D

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.

TODO:

• Create a "unit test"-like scene

[ramokz]

This looks very promising!

Thanks for putting this up so quickly and starting the work on it.

To note am not overly familar with C# implementation in Godot and am still reading up on the general guidelines and approaches for it, so take the thought below with a grain of salt.

Wonder if we should be using the non-abbreviated name for the class names? So instead of PCam3D.cs it would be PhantomCamera3D.cs. Think when referring to the namespace in other C# scripts, it would make it easier to read and align with what seems to be the convention of using the whole name as pascal cased.
Considering that you wouldn't call the PCam2D/3D classes directly, as they have no functionality in of themselves, but rather being used to define node types. So the shortening of the name shouldn't matter much. In fact, it would align better with how it's done in GDScript where you assign a type using var pcam_3d: PhantomCamera3D. So the class declaration would look more like:

public class PhantomCamera3D : PhantomCamera

Related to the above, wonder if we can use the PhantomCamera.cs as an interface class for the PhantomCamera2D and PhantomCamera3D classes. Think it would mean that the user wouldn't have to import both PhantomCamera and PhantomCamera3D (or currently PCam3D) namespaces in every C# file they're needed in. It should also reduce the amount of properties and functions needed in the 2D and 3D variant as there are a fair bit of overlap. I wanted to do it that way in GDScript, and did try pre-0.7, but it's one of those annoying shortcomings of the language — unlike with C#. Obviously, don't worry about a 2D version or having all the relevant properties in an interface for this PR, but mainly thinking of the general structure for how to leverage the benefits of the language,

Also related, wonder if this should act like just a class you assign to a given PCam2D/3D node instead of a Godot Extension, though not sure if it actually is(?). Looking at some other addons that have C# bindings, they seem to just have a general class that has the propeties, methods, signals etc. as part of it that you just either instantiate as a new node, type assign to an existing nod, or, like here, call methods from directly. Been mainly looking at examples such as dialogue_manager and better-terrain. better-terrain seems like the better comparison of the two.

[sircodemane]

@ramokz thanks for the feedback, and no worries, I'm totally new to Godot and C# is not my main language by any means, so I don't have any strong opinions 😅

Wonder if we should be using the non-abbreviated name for the class names? So instead of PCam3D.cs it would be PhantomCamera3D.cs. Think when referring to the namespace in other C# scripts, it would make it easier to read and align with what seems to be the convention of using the whole name as pascal cased.

I had considered this as well and I think I originally used the shorthand to avoid collisions before realizing that wasn't actually an issue. In hindsight, I agree with you here, let's use the full names.

Related to the above, wonder if we can use the PhantomCamera.cs as an interface class for the PhantomCamera2D and PhantomCamera3D classes. Think it would mean that the user wouldn't have to import both PhantomCamera and PhantomCamera3D (or currently PCam3D) namespaces in every C# file they're needed in. It should also reduce the amount of properties and functions needed in the 2D and 3D variant as there are a fair bit of overlap.

In C#, as long as each file is defined under the same namespace, it treats them all as a package. So in my project, I only had to import the single namespace using PhantomCamera; and it gives me access to the PCam3D, Extension, enums, etc without having to import each thing individually.

That being said, I hadn't looked much at the 2D implementation yet, but with a good amount of overlap, I also agree that having a base interface to reduce redundancy is the way to go.

Also related, wonder if this should act like just a class you assign to a given PCam2D/3D node instead of a Godot Extension, though not sure if it actually is(?). Looking at some other addons that have C# bindings, they seem to just have a general class that has the propeties, methods, signals etc. as part of it that you just either instantiate as a new node, type assign to an existing nod, or, like here, call methods from directly. Been mainly looking at examples such as dialogue_manager and better-terrain. better-terrain seems like the better comparison of the two.

These are really good examples. One thing I had really hoped to do is make it so a user could cast a Node3D into a PhantomCamera like with any C#-based nodes:

var pCam3D = GetNode<PhantomCamera3D>("path/to/pcam3d");

I wanted this so there wasn't a clear disconnect between a PhantomCamera object and the underlying node that drives it (for example, not being able to call normal Node3D properties or methods on it) but I ran into a number of issues trying this. However, DialogueManager seems to derive from Node so I'm gonna study what they did and have another go at it. I'd like for users to use this like any other c# Node and not have to do any gymnastics around it.

A few other questions I had:

• Should we commit the C# project files? I normally would, but since we ignore the godot project file I just followed that.
• I've got a few imports that keep wanting to update in my branch, any guidelines for handling godot dev assets?
• Do you have any specific guidelines related to testing/example scenes with C#? I did want to create some test scenes at minimum

[sircodemane]

Second pass is up. Changes:

• Moved everything to single file at top of the addon folder, mimicking other projects
• includes:
  • GodotExtension (extension methods that add Node#D.AsPhantomCamera#D())
  • PhantomCamera base abstract class
  • PhantomCamera2D
  • PhantomCamera3D
  • Enums (FollowMode, LookAtMode, InactiveUpdateMode)
• shares logic
• almost all of the public API is hooked up except for the Tween resource and Camera resource

[ramokz]

These are really good examples. One thing I had really hoped to do is make it so a user could cast a Node3D into a PhantomCamera like with any C#-based nodes:

var pCam3D = GetNode<PhantomCamera3D>("path/to/pcam3d");

I wanted this so there wasn't a clear disconnect between a PhantomCamera object and the underlying node that drives it (for example, not being able to call normal Node3D properties or methods on it) but I ran into a number of issues trying this. However, DialogueManager seems to derive from Node so I'm gonna study what they did and have another go at it. I'd like for users to use this like any other c# Node and not have to do any gymnastics around it.

100% agree. That would be the ideal approach to have DX alignment between working in GDScript and C#. Reading up on this a bit further, I don't believe it's possible when doing a non-GDExtension addon, like this one, to have both C# and GDScript recognize custom classes written in either language. I.e. C# doesn't see a PCam3D node as a PhantomCamera3D class that extends Node3D, but rather just a Node3D as a Node3D class with a script attached to it. I would love to be proven wrong, however!

Related, I wasn't sure at first what the code here was doing:

public static class GodotExtension  
{  
    public static PhantomCamera3D AsPhantomCamera3D(this Node3D node3D)  
    {        return new PhantomCamera3D(node3D);  
    }  
    public static PhantomCamera2D AsPhantomCamera2D(this Node2D node2D)  
    {        return new PhantomCamera2D(node2D);  
    }}

But digging a bit further about how Godot does GDScript -> C# cross-language class referencing, or rather the lack thereof, I can see how it makes sense and works with these two lines:

var pCamNode = GetNode<Node3D>("Player/PlayerCam");  
var pCam = pCamNode.AsPhantomCamera3D();

In the grand scheme of things, i.e. one extra line per PCam reference, it feels like a good compromise considering the seeming technical limitations at play.

---

• Should we commit the C# project files? I normally would, but since we ignore the godot project file I just followed that.

Do you mean the .sln and .csproj files? If so, then I will say no. Think that is generated when people make a new Godot C# project and contains data that should be project specific anyhow.

---

• I've got a few imports that keep wanting to update in my branch, any guidelines for handling godot dev assets?

What imports do you keep seeing? Don't seem to get anything usual aside from typical .idea/ files.

---

• Do you have any specific guidelines related to testing/example scenes with C#? I did want to create some test scenes at minimum

Think that would be very useful for testing / debugging. Adding a test scene file in the /dev_scenes/3d, like you've already done, makes sense. Don't think there's a need for a C# version of the example scenes, as that would just be extra maintenance work without any real benefits. The documentation site ought to give clear enough instructions about how to use C#, as it's only the code that would differ, rather than the scene structure. But that's a post-PR concern.

The only comment I have for the scene you've made already is that it would be ideal to reuse as much of the existing GDScript files that doesn't directly involve the addon's logic. So rather than making a C# version of the player movement script, it can just be the same GDScript (res://addons/phantom_camera/examples/scripts/3D/player_controller.gd) as used in the non-C# dev / example scenes.
Note: there are some scene-specific setup requirements for that player script in question.

[ramokz]

A small thought about the location of the main wrapper file, would it cayse any issues by having the PhantomCamera.cs inside the /scripts/phantom_camera/ directory? Think it would keep it a bit tidier having the addon code files in a single folder, rather than at the top-level. Likewise, I would place a future PhantomCameraManager.cs wrapper file inside the scripts/phantom_camera_manager/ directory.

[sircodemane]

100% agree. That would be the ideal approach to have DX alignment between working in GDScript and C#. Reading up on this a bit further, I don't believe it's possible when doing a non-GDExtension addon, like this one, to have both C# and GDScript recognize custom classes written in either language. I.e. C# doesn't see a PCam3D node as a PhantomCamera3D class that extends Node3D, but rather just a Node3D as a Node3D class with a script attached to it. I would love to be proven wrong, however!

Yea it's definitely not possible lol. I'm not really sure why DialogueManager derives from Node since they don't get anything from it, but yea, the C# type extension was the most ergonomic choice.

Do you mean the .sln and .csproj files?

Yep. I noticed other projects have them commit to the project root, which seems fine (and it's standard to commit those files in c# projects), as long as we're not polluting the addons folder since that seems to be what gets packaged. I'll keep them ignored for now, unless you change your mind about it.

What imports do you keep seeing?

These two files popup every time and I just roll them back before committing:

modified:   addons/phantom_camera/examples/textures/3D/checker_pattern_dark.png.import
modified:   addons/phantom_camera/icons/phantom_camera_gizmo.svg.import

Think that would be very useful for testing / debugging ...
The only comment I have for the scene you've made already is ...

Awesome, will do. I also plan to create a unit-test-like scene that should allow for running the scene to execute a series of tests to check for regressions when making future updates to the gdscript/api

A small thought about the location of the main wrapper file, would it cayse any issues by having the PhantomCamera.cs inside the /scripts/phantom_camera/ directory?

That shouldn't be an issue. I also hadn't consider the Manager, I'll set that up as well. The only opinion I have on that is it should live in the same directory (or even the same file) as PhantomCamera.cs unless you want it to be in a different namespace. It's not a requirement, but it is standard to have one directory per namespace (and vice versa).

[sircodemane]

Ok, new update: I think I have finished the whole API (at least from what I could find). For now everything is in the PhantomCamera.cs file. It's a little beefy, but not too unwieldly. I still need to write tests to cover everything, so I'll be working on those this evening. I also updated the original PR comment to encompass all the changes. Let me know what you think!

[ramokz]

Thanks for updating the original comment with an easy-to-read overview. Think it covers everything very succinctly.
The wrapper file is also looking fantastic and is really well put together!

Yep. I noticed other projects have them commit to the project root, which seems fine (and it's standard to commit those files in c# projects), as long as we're not polluting the addons folder since that seems to be what gets packaged. I'll keep them ignored for now, unless you change your mind about it.

Ah apologies, for some reason thought it was for the addons/phantom_camera directory for some reason.
Yes, for the project root, that should be fine to add them there. People who forked the project will likely need those files as well.

These two files popup every time and I just roll them back before committing:

This is a problem that I've seen some other folks have had before. But some also don't encounter this — myself included.
It's unrelated to this PR, and I'm not sure why Godot is a bit loosey-goosey with import files like this. The exact changes seem trivial, so feel free to submit them as well.

Awesome, will do. I also plan to create a unit-test-like scene that should allow for running the scene to execute a series of tests to check for regressions when making future updates to the gdscript/api

That would amazing to have! Honestly, I am not overly familiar with setting up unit-tests, so any support with that would be greatly appreciated. It's something I have considered as the project has been growing over time, just never found a good time to explore it.

That shouldn't be an issue. I also hadn't consider the Manager, I'll set that up as well. The only opinion I have on that is it should live in the same directory (or even the same file) as PhantomCamera.cs unless you want it to be in a different namespace. It's not a requirement, but it is standard to have one directory per namespace (and vice versa).

Personally, I think it's fine for it to sit in the same file and namespace. So leaving it inside the Scripts directory, like you've done already, sounds sensible to me with the addition of PhantomCameraManager being included in the file now too.
Did go back and forth about whether if it would be more manageable if the script was split into separate files, e.g. PhantomCamera3D, PhantomCameraManager etc. But given the file isn't overly large, I think it simplifies the general usage by having it in one file. Though I can imagine it's a highly subjective matter, at the end of the day.

I also verified an export built with template flag disable_3d=true, and no issues arose, which was my main concern by having everything in one file for 2D projects with that build flag.

---

One small error I'm encountering in 2D scenes is in relation to the signal LookAtTargetChanged, which is a PCam3D only signal. So I'm guessing it would need to be applied as an override property within the PhantomCamera3D class. It doesn't break anything; however, it is a persistent error log when using the class in 2D scenes.

Another thing I noticed is that accessing PhantomCameraManager, or more specifically the arrays within it, appears not to return a valid reference. Getting this error:

NativeCalls.cs:3666 @ Godot.GodotObject Godot.NativeCalls.godot_icall_1_427(nint, nint, Godot.NativeInterop.godot_string_name): Failed to retrieve non-existent singleton 'PhantomCameraManager'.

When I reference PCam2D nodes using PhantomCameraManager.PhantomCamera2Ds;

I did see that dialogue_manager uses Engine.GetSingleton as well, so am not sure why it doesn't work here. Will try to investigate a bit to see what it might be.

[sircodemane]

That would amazing to have! Honestly, I am not overly familiar with setting up unit-tests, so any support with that would be greatly appreciated. It's something I have considered as the project has been growing over time, just never found a good time to explore it.

This is actually turning out to be a bit more difficult that I thought 😅 I started writing my own little test runner suite and then decided to try GdUnit. GdUnit is really cool, but does add a number of dev dependencies and I've not had much luck getting the SceneRunner to function correctly. I may revert back to my own version, as that at least worked without issues.

One small error I'm encountering in 2D scenes is in relation to the signal LookAtTargetChanged, which is a PCam3D only signal. So I'm guessing it would need to be applied as an override property within the PhantomCamera3D class. It doesn't break anything; however, it is a persistent error log when using the class in 2D scenes.

I will look into this!

Another thing I noticed is that accessing PhantomCameraManager, or more specifically the arrays within it, appears not to return a valid reference. Getting this error:

I actually ran into this same issue once I started writing tests. I believe the reason Engine.GetSingleton() works for dialogue_manager is because of this little bit of logic: https://github.com/nathanhoad/godot_dialogue_manager/blob/main/addons/dialogue_manager/dialogue_manager.gd#L83

# Make the dialogue manager available as a singleton
if Engine.has_singleton("DialogueManager"):
	Engine.unregister_singleton("DialogueManager")
Engine.register_singleton("DialogueManager", self)

I meant to ask about doing this for the PCamManager, but I got caught up trying to get GdUnit working so it's been sitting in my Todo list lol

[sircodemane]

I've reverted back to my hand-made test suite since it just works ™️ . I also fixed the look at target change signal and added the Engine singleton registration, which I have confirmed does fix the Engine.GetSingleton call, so now PhantomCameraManager can function properly as a static singleton. I also started implementing tests, but need to call it a night. If the testing framework looks alright, I'll add a readme to explain it for any future contributors.

[ramokz]

This is actually turning out to be a bit more difficult that I thought 😅 I started writing my own little test runner suite and then decided to try GdUnit. GdUnit is really cool, but does add a number of dev dependencies and I've not had much luck getting the SceneRunner to function correctly. I may revert back to my own version, as that at least worked without issues.

There is an argument to be made that unit-testing could be its own PR, as it doesn't directly affect this feature addition.
Think it might also make it easier to keep track of from a main branch commit point-of-view in case something goes awry.
I generally try to keep commits / PRs as specific as possible.

I meant to ask about doing this for the PCamManager, but I got caught up
trying to get GdUnit working so it's been sitting in my Todo list lol

Good spot! Think it should be fine to add that in. Runs well from my tests as well.

The signal error has also been resolved. Thanks for the quick fix!

[ramokz]

Does this need to be unregistered first?
Wonder if it could just be a case of adding the singleton if it doesn't exist — basically, inverting the conditional checker and adding the Engine.register_singleton inside of it.

[sircodemane]

There is an argument to be made that unit-testing could be its own PR, as it doesn't directly affect this feature addition.
Think it might also make it easier to keep track of from a main branch commit point-of-view in case something goes awry.
I generally try to keep commits / PRs as specific as possible.

Normally I would advocate for new tests being added as part of a PR with new features, but since in this case we're also introducing a testing framework and setting the stage for future testing, there's definitely some discussion around that. I'll pop an issue with some prelim info later today and split this PR up. The testing can be a fast-follow PR if you're ok with that.

Does this need to be unregistered first?

To be totally honest, I don't know the answer. I just mimicked what dialogue_manager was doing because I don't know what semantics might be in play. My assumption is that maybe there's a case where the Manager node can get replaced somehow, and unregistering any old references and replacing with the current one is just a safety measure, but that goes beyond my understanding. I don't mind inverting the conditional if you think we should, let me know what you think. Thanks for all your feedback!

[ramokz]

Normally I would advocate for new tests being added as part of a PR with new features, but since in this case we're also introducing a testing framework and setting the stage for future testing, there's definitely some discussion around that. I'll pop an issue with some prelim info later today and split this PR up. The testing can be a fast-follow PR if you're ok with that.

Think a fast-follow PR sounds like a sensible approach, so am happy to go with that.

To be totally honest, I don't know the answer. I just mimicked what dialogue_manager was doing because I don't know what semantics might be in play. My assumption is that maybe there's a case where the Manager node can get replaced somehow, and unregistering any old references and replacing with the current one is just a safety measure, but that goes beyond my understanding. I don't mind inverting the conditional if you think we should, let me know what you think. Thanks for all your feedback!

Looking briefly at Godot's docs about it, it appears not to do anything special beyond the obvious. In fact, it specifically mentions, “The singleton object is not freed.”. So can't see what purpose it has in dialogue_manager aside preventing the singleton from being registered twice to avoid an error log. So think it should be safe to simplify it by just registering the singleton if it isn't already.

[sircodemane]

Added a new update. I've continued adding tests for the sole purpose of making sure things work and I've fixed numerous bugs along the way 😅

I also split the files how you suggested previously, the single file had reached a point of becoming quite a burden. I also inverted the singleton registration like we discussed.

Also changed how Get/SetLimitTarget in PhantomCamera2D.cs worked so users could work with TileMaps and CollisonShape2Ds more directly. I originally coded it as a NodePath, which was opaque and error prone.

I also realized I've missed quite a bit of the API related to specific modes, so I gathered that list and TODO'ed them for myself to pick up tomorrow. One question on this: If the user interacts with a property incompatible with the current settings, should we throw an exception in C#? example- changing dead_zone values when not using FollowMode.Framed

Once I get things all squared away I will pick the testing changes out into a separate PR and clean this one up before taking it out of draft status. All in all, I'm really happy with how well the C# API is shaping up! Looking forward to getting it all done soon.

[ramokz]

Also changed how Get/SetLimitTarget in PhantomCamera2D.cs worked so users could work with TileMaps and CollisonShape2Ds more directly. I originally coded it as a NodePath, which was opaque and error prone.

That is a good shout. I did want to do the same in the GDScript file, but it doesn't allow for specifying two nodes types as an @export variable, which is the only reason why NodePath is used here.

If the user interacts with a property incompatible with the current settings, should we throw an exception in C#? example- changing dead_zone values when not using FollowMode.Framed

It would help with user comprehension if the code threw a warning / exception if they were making a property change that had no effect on the PCam. The GDScripts does do this in a few places already, but not everywhere. My understanding is that the C# script is merely calling the methods from the GDScript? If so, think it would make more sense to add the warnings / exceptions to the original GDScripts to centralise it. As opposed to having to either maintaining the same validators in two places or there being a discrepancy between the two languages. To be clear, don't worry about any changes to GDScript for this PR.

[sircodemane]

Hi @ramokz just checking in since it's been a while. I need to take a break for a bit as I was burning myself out, but I'm looking to get back to it this week. Hope all's well!

[ramokz]

Hey, no problem at all! Experienced burnout myself in the past and wouldn't wish it upon my worst enemy.
The work in this PR is stellar and would hate to rush it at the cost of mental health, so, honestly, take as long as you feel you need 🙂

[SimoneRizzuto]

Hi, this work looks really nice!

I see you only need to make a "unit test like scene".
Is it possible to get this merged into main anyway, even without the unit test related work?

I ask, because I use C#, and the wrapper would be very useful right now as I am attempting to port my game to use the Phantom Camera library to fix jittering issues on my end (hopefully).

Absolutely no pressure if you don't feel comfortable doing it, I am simply requesting as it would be very nice.

[sircodemane]

Hi @SimoneRizzuto I can take responsibility on this one 😅

I wouldn't feel comfortable merging this as is because I would not consider it stable. As I have been writing tests, I've found numerous problems in how I was connecting gdscript objects to C# and finding that things did not work as I had assumed (yay tests lol). I have also found things that I missed, which adds more tests, and while we have decided to introduce testing as a separate PR, it has been very valuable in stabilizing this work.

All that being said, I am setting aside time this weekend to try and get this wrapped up as it has been languishing for quite some time now.

[SimoneRizzuto]

@sircodemane
Alright, that's awesome to hear. I'll keep my eye on this PR over this weekend.
I would offer some help, but I haven't the faintest idea how any of this works. 😅
Thanks a lot though! :D

Maybe I'll truck along on another feature of my game in the meantime.

[ramokz]

@sircodemane as an aside, don't feel pressured to have it wrapped up at any specific point in time. Ultimately, this is an entirely voluntary effort, and so will always favor people taking the time they feel they need if life demands time/energy elsewhere.

[SimoneRizzuto]

Yeah, agreed.

[sircodemane]

Heyo, I didn't quite have the time I'd hoped to work on this, but I was able to dive back in and start getting my bearings again. I've started looking at changes in upstream to see what updates I need to make to be compatible. I expect to have more updates throughout this week. Thanks yall! ❤️