Skip to content
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

Omit the &self parameter for functions on engine singletons #127

Open
ttencate opened this issue Feb 15, 2023 · 7 comments
Open

Omit the &self parameter for functions on engine singletons #127

ttencate opened this issue Feb 15, 2023 · 7 comments
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals

Comments

@ttencate
Copy link
Contributor

Instead of Input::singleton().is_key_pressed() you should just be able to write Input::is_key_pressed(). It's shorter, closer to GDScript, more Rusty, and avoids the overloaded word "singleton" which can also refer to an autoload.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: engine Godot classes (nodes, resources, ...) labels Feb 15, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 15, 2023

The problem with this is that you can no longer access the actual singleton object.

A few drawbacks:

  1. We lose object properties. Gd::instance_id() etc. are no longer accessible. Not sure why anyone would need that, but still.
  2. We lose the class hierarchy. You can do Input::singleton().call("dynamic_method"), but this won't work as Input::call("dynamic_method").
  3. You can no longer differentiate between mutating and non-mutating methods. E.g. Input::add_joy_mapping is marked as is_const=false. This matters if someone wants to store the temporary singleton() object in a variable, or wants to pass &Input references to functions that only perform read operations.
  4. Caching is more complex. Right now, a user can simply store a Gd<Input> somewhere and thus save global_get_singleton FFI calls. If Input is global, the singleton must either be loaded at startup (for all singleton classes) or loaded lazily.
  5. It may cause slight confusion because this syntax is used by static methods in other places. While conceptually the same, the distinction matters e.g. when using reflection/introspection.
  6. It will still be possible to receive Gd<Object> objects from GDScript which dynamically point to the Input singleton. However, we cannot call any methods on it (except through reflection).

I don't think either of those points is a roadblock, but it's important to be aware that the proposal, while syntactically nicer, has quite some implications.

We can probably still try it. We might even want to start without providing an object at all.

@bluenote10
Copy link
Contributor

We can probably still try it. We might even want to start without providing an object at all.

Or perhaps we can even have both? Input::singleton() in case e.g. someone wants to pass it around read-only, but also free-floating functions (maybe in an accompanying module) so that one can write input::is_key_pressed() for convenience? A drawback of having both would of course be the documentation duplication, and the increased total interface.

@ttencate
Copy link
Contributor Author

Thanks for the detailed reply. It's good to be aware... but yeah, I don't think any of these are blockers:

  1. If anyone ever needs any of these methods, we can implement them as static wrappers too.
  2. Currently, all 35 singletons inherit from Object.
  3. Why would anyone want to store the singleton in a variable or pass a reference to it?
  4. Caching becomes an implementation detail of the gdextension library, so users don't need to worry about it. Someone on Discord mentioned that we can cache function pointers in a lock-free way using a static AtomicPtr; the same technique could work to cache the instance pointer.
  5. Not sure I follow, since Rust doesn't have reflection.
  6. Why would anyone want to do that?

@Bromeon
Copy link
Member

Bromeon commented Feb 15, 2023

Thanks as well!

  1. Yes, or see below 🙂
  2. Oh, thanks for checking! That would mean any functionality would be limited to Object methods, which makes things easier.
  3. I guess it would be a very niche use case if someone insists on the mutability-check. But even then, the function can always get a fresh singleton pointer globally and mutate that.
  4. With only 35 singletons, it's probably not needed to be smart about this -- we can just load all instance pointers at global init and store them in a global array. Similar to what we do with function pointers. Reading pointers concurrently is safe.
  5. Godot has reflection though. Object::call() or Object::emit_signal() are dynamic function calls. Some APIs take a self receiver, and I can imagine they accept null for static methods, but an actual object for singleton method calls. E.g. the Callable(obj, method) constructor. To be verified though.
  6. I'm just demonstrating that it's still possible to have Gd objects through backdoors. Which also means some pitfalls like .free() need to handle this (there's a TODO).

I think what we could do is

impl Input {
    pub fn singleton_object() -> Gd<Object>;
}

to still give access to the underlying instance as Object (it could be Gd<Input>, but there's no extra API).


Another topic we need to clarify, is thread safety. The latest threading guidelines specify:

Global scope

Global Scope singletons are all thread-safe. Accessing servers from threads is supported (for RenderingServer and Physics servers, ensure threaded or thread-safe operation is enabled in the project settings!).

This makes them ideal for code that creates dozens of thousands of instances in servers and controls them from threads. Of course, it requires a bit more code, as this is used directly and not within the scene tree.

Which sounds quite promising! So is every possible method synchronized, and we don't need to worry about anything, ever? 🤩

Because if not, it's going to be painful to model in safe Rust...

@Bromeon
Copy link
Member

Bromeon commented Feb 15, 2023

Or perhaps we can even have both? Input::singleton() in case e.g. someone wants to pass it around read-only, but also free-floating functions (maybe in an accompanying module) so that one can write input::is_key_pressed() for convenience? A drawback of having both would of course be the documentation duplication, and the increased total interface.

Yeah, I'd rather not duplicate APIs for syntactical reasons. But the const-ness is anyway not that helpful, as people can always just get a new Gd pointer, from anywhere (globally accessible). As such, I think the approach in my last post could work reasonably well.

@Bromeon
Copy link
Member

Bromeon commented Aug 10, 2023

Another thing, there are cases where Godot might return singleton objects (even if it's legacy/compat code), and we must be able to represent them somehow.

godotengine/godot#75694 (comment)

TLDR: GDExtension changes the EditorInterface class from a normal class into a singleton class.
They deprecate get_editor_interface() and use a globally accessible EditorInterface instance instead.

There are two problems with this proposal in particular:

  1. In GDExtension, modifying "singleton-ness" of a class is not a breaking change.
    • In particular, dsnopek's comment:

      This won't break binary compatibility and we never really promised that singletons couldn't be passed as instances.

    • Under the proposal in this PR, this would become a breaking change in Rust.
    • This happens sufficiently rarely that breaking changes are imo OK, but we risk missing them and later violate SemVer.
  2. What would get_editor_interface() return under this proposal? Gd<EditorInterface> would be outlawed, no?

@ttencate
Copy link
Contributor Author

We could still allow Gd<T> to please the compiler, maybe also keep T::singleton() around, but the T would have no useful methods on it since they're all static.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

No branches or pull requests

3 participants