Object notification API, engine intra-doc links by Bromeon · Pull Request #223 · godot-rust/gdext (original) (raw)

Thanks for the feedback!

What is unfortunate is that the virtual method NodeVirtual::notification (receiver) would then have the same name as notification (sender) in GDScript. Since trait methods are in scope when defining a custom struct, someone may do this:

#[godot_api] impl MyClass { fn stuff(&mut self) { // Send a notification to the tree self.notification(NodeNotification::PhysicsProcess); // BUG: completely bypassed Godot, just called the Rust method directly } }

A on_ prefix makes clearer that it's a callback in this case. I think it's less of a problem for ready(), process() etc, as they have more specific names and are well-known for any Godot developers, but in theory we could also consider on_ prefixes for those.

For the same reason I wouldn't rename the sending method, except that now we have a naming conflict if both are called notification. Maybe send_notification instead of issue_notification? Shorter and clearer. Or consider notify, which is consistent with SceneTree::notify_group{,_flags}.

"send" is not great because other such occurrences in Godot are networking- or server-related.

"notify" sounds good. I was considering it as well, but thought it might not be clear enough. I didn't think of notify_group though, thanks!

I like type safety as much as anyone, but isn't using enums overly constraining here? What happens if I call notify_group and this results in a Node2D receiving a notification that is defined only for Node3D? I see you've added an Unknown(i32) but that seems like a bit of a code smell. In practice, people will always use a non-exhaustive match with an _ arm, so what is the added value of an enum instead of a set of i32 constants?

There is no easy way to know the NOTIFICATION_* constants that are relevant for any given class.

If we consider Node3D as an example, we have such constants spread over Node3D, Node and Object. Unlike GDScript, Rust does not inherit constants, so auto-completing Node3D::NOTIF... will not give an exhaustive list of constants. So a user would need to manually dig through the docs of all relevant base classes.

And even if Rust did inherit constants (or duplicate them), we'd still have the problem that a user would not be informed about additions in Godot. If I have an enum, a match branch will force me to list all the relevant constants. If Godot 4.1 now adds NOTIFICATION_HOUSE_ON_FIRE, this means my code must be updated to account for it, because it doesn't fall into the Unknown(...) branch. I can always use _ if I don't care about others. (This is generally a double-edged sword; what can be a feature for type safety may be in the way of forward-compatibility. So there's no clear "better").

Then there's also the self-descriptiveness. Numbers are terrible for printing, debugging, etc; whereas enumerators can derive Debug to have a descriptive string representation. Possibly this could be addressed with constants as well, by defining them as something like:

pub const NOTIFICATION_XY: Constant = Constant(73, "XY");

Last, there's also type safety. You cannot accidentally send a CanvasItem notification to a Node3D receiver.

But yeah, maybe some of the parts are overkill... notify_group also doesn't offer this level of type safety, however by its nature it can't (dynamic reflection call).