Some NOTIFICATION_* constants collide by sharing the same integer value
#75839
Labels
Milestone
Comments
bors bot
added a commit
to godot-rust/gdext
that referenced
this issue
Apr 19, 2023
223: Object notification API, `engine` intra-doc links r=Bromeon a=Bromeon
So far, there was no way to intercept the Godot `_notification` API, and also issuing notifications was very cumbersome, as the constants were spread across several classes.
This PR introduces multiple new APIs:
1. `{Class}Notification` enum, which contains all notifications relevant for a given class.
* For example, `Node3DNotification` accumulates all `NOTIFICATION_*` constants across `Node3D`, `Node` and `Object` classes.
* In GDScript, this is less of an issue because there is true inheritance, so one can type `Node3D.NOTIF...` and auto-complete will list all candidates. In Rust, constants are not inherited.
* There is currently a workaround for bug [godot#75839](godotengine/godot#75839): in two cases, two enumerators cannot be distinguished as they share the same integer value. We address this by using a single enumerator to represent both (not ideal, but there's nothing else we can do).
2. Virtual method `{Class}Virtual::on_notification(what: {NearestClass}Notification)`.
* This is the equivalent to GDScript's `_notification` (with underscore), but more meaningfully named. It can be used to intercept low-level notifications for the given class.
* The parameter type is either the notification enum for that class; or if the class itself does not have any notification constants, the nearest base class that _does_. This avoids proliferation of enum types with the exact same interface. It's possibly breaking when Godot adds such constants in new classes, but it helps reduce the API surface if not _every single class_ needs its own enum (since there are relatively few classes working with notifications).
3. The method `{Class}::issue_notification(what: {NearestClass}Notification)`.
* This is the counterpart of `on_notification` to _send_ notifications.
* In GDScript, this is named `notification` (no underscore).
---
Apart from that, there are smaller changes:
* `Display` is now implemented on all `Gd<T>` types, by using the Godot string representation. `T`'s `Display` impl is ignored.
* Several generated symbols in `godot::engine` are now slighly documented and contain intra-doc links to navigate easily:
* Class <-> virtual trait
* Class <-> notification enum
* Class <-> sidecar module
* Class -> Godot online docs
* Modules `global`, `utilities` and `notify`
Co-authored-by: Jan Haller <bromeon@gmail.com>
|
The other duplications are less problematic, as they appear in disjoint inheritance branches. Is the decision to keep them as-is? |
|
I think so. For 5.0 we should look into a system that avoids such overlaps, but for now it would be worth it IMO to break compat in those key notifications if there's no actual problem with reusing the same number in separate inheritance chains. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Godot version
4.0.2-stable
System information
N/A
Issue description
Godot uses
NOTIFICATION_*constants across different classes to identify different types of lifecycle callbacks. Since they are defined in different classes, they are not enums, but resolve tointin GDScript. At the moment, some of these constants share the same value, although they mean different things.Examples I've found (not necessarily exhaustive):
It can be argued that this may not be an issue as long as those collisions happen in different inheritance branches (i.e. never directly between a base and derived class).
However, the first example above even violates that: both
CanvasItemandWindowinherit fromNode, so there are simultaneously two different notification constants of the same value in scope.This can be problematic in the virtual function
Object::_notification(int what), which can be overridden by the user. There is no way to distinguish such notifications at the moment.The open question is whether only this specific bug should be fixed, or collisions should be avoided in general. Given a 32-bit-integer, it's probably reasonable to never have overlapping values; yet backwards-compatibility (mostly with GDExtension) might be a factor in the decision.
Steps to reproduce
See above.
Minimal reproduction project
N/A
The text was updated successfully, but these errors were encountered: