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

Reintroduce get_ prefixes (revert #477) #487

Merged
merged 2 commits into from
Nov 19, 2023
Merged

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Nov 18, 2023

This reverts commit d36a144 from pull request #477.
Prepares some logic for overriding const-qualification, without yet enabling it.

Rationale:

  1. Godot has non-typical APIs for Rust, following tradititional OOP style with lots of getters/setters.

  2. My idea was that porting GDScript's property access to Rust might be more natural: node.position becomes node.position(). However, Godot's naming is much less consistent than I was initially assuming:

    • size_flags_horizontal property has {get/set}_h_size_flags accessors
    • Node.get_path() and Node.get_parent() look like they should have properties, but don't; so .path() and .parent() would be a "wrong mapping".
    • Node bool property unique_name_in_owner has {get/is}_unique_name_in_owner.
      However, {get/set}_scene_instance_load_placeholder methods also return bool but have no property. So mapping would yield is_unique_name_in_owner and scene_instance_placeholder. Stripping the "is_" for consistency would be confusing: unique_name_in_owner sounds like it returns the name.
  3. Some get_ methods take parameters.

    • MeshDataTool has get_vertex_count() and get_vertex_color(index) -- the former would lose its prefix but the latter would not since it's not strictly a "getter" in the Rust sense.
    • It could even happen that such parameters have defaults, in which case a single method would act as both getter and getter-with-param. It's unclear how we'd map it under our _ex model.
  4. Since some verbs are simultaneously nouns, this makes APIs slightly harder to understand in some cases.

    • scale()
    • queue() (already causing a conflict)
    • process_delta_time()
    • format()
    • modulate()
  5. Exceptions and rules that depend on presence of properties make backwards compatibility harder.

    • if a new property is added, it might not map to the way how Rust maps an existing get_ method
    • if Godot has a method get_format() which we map to format(), and then Godot adds another method format() later, we have a conflict
  6. Getters are harder to discover.

    • partly due to issues above (intermixing with "doer" style methods/verbs)
    • there is no more get_ autocompletion in IDEs

TLDR: There needs to be a complex ruleset, with lots of edge cases. In the end we may come up with something that looks nice and somewhat consistent when looking at the Rust part, but it will be too distant from the GDScript API, making porting between the two harder. We should probably embrace imperfections in the Godot API rather than trying to fight them.

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

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-487

This reverts commit d36a144.
Prepares some logic for overriding const-qualification, without yet enabling it.

Rationale: see PR.
@Lamby777
Copy link
Contributor

Is there some consideration for having this naming convention for other stuff like get_singleton instead of singleton? Or is that different because it's just an associated function and not a method?

@Bromeon
Copy link
Member Author

Bromeon commented Nov 19, 2023

In the docs link by the bot posted before your message, you'll see that Engine::get_singleton still exists:
https://godot-rust.github.io/docs/gdext/pr-487/godot/?search=get_singleton

[Edit] link will become garbage-collected after this has been merged, you can check master docs though.

The other ones, to access the singleton instance of a certain class, will remain being called singleton (that's unchanged). This method will no longer be needed (or get a niche function) when we tackle #127, though.

@Bromeon Bromeon force-pushed the qol/revert-get-prefixes branch 2 times, most recently from 2e75b2f to 749d97d Compare November 19, 2023 09:26
@Bromeon Bromeon added this pull request to the merge queue Nov 19, 2023
Merged via the queue into master with commit d96906f Nov 19, 2023
15 checks passed
@Bromeon Bromeon deleted the qol/revert-get-prefixes branch November 19, 2023 09:47
@Bromeon
Copy link
Member Author

Bromeon commented Nov 19, 2023

Thanks a lot for the fast and well-argued feedback on this change, especially to @bluenote10! 👍

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

Successfully merging this pull request may close these issues.

3 participants