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

Add OnEditor<T>, remove impl<T> Export for Gd<T> and DynGd<T, D> #1051

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Feb 16, 2025

Closes #892

  • Add OnEditor<T>.
  • Add before_ready check for OnEditor fields to panic and warn user in case if values haven't been set.
  • Remove impl<T> Export for Gd<T> and DynGd<T, D> - it should be provided by algebraic types instead.
  • FIX: Option<Gd<T: Inherits<Node>>> and OnEditor<Gd<T: Inherits<Node>>> can no longer be used as an Export for Resource-based classes.

Draft, since I have to give it a proper test run in real-world environment.

@Yarwin Yarwin added feature Adds functionality to the library c: core Core components c: register Register classes, functions and other symbols to GDScript breaking-change Requires SemVer bump labels Feb 16, 2025
@Yarwin Yarwin marked this pull request as draft February 16, 2025 15:46
@Bromeon Bromeon added this to the 0.3 milestone Feb 16, 2025
@Yarwin
Copy link
Contributor Author

Yarwin commented Feb 16, 2025

Note: I messed around with a bit more sophisticated init and whatnot and IMO it is overshoot; OnEditor has only two states – HasValue and Invalid thus there isn't really need for anything more than that

After some tweaking I decided to support (some) built-ins as well.

Alright, long story short, this PR removes implementation of the Export trait for Gd and DynGd in favor of exporting/expressing these with algebraic types – Option<…> and OnEditor. OnEditor primarily expresses a nullable property that can be set via Godot Editor and should not be null on the runtime. For now it does not provide elaborate init logic, but it might be changed in the future – albeit I would rather avoid that, to make Gd<Resource> support more intuitive and straightforward.

Additional logic is being run before ready for node-based classes which informs the user which properties haven't been set.

I could not find proper blanket implementation that would satisfy all the GodotTypes, so we are stuck with three different impls – blanket implementations for Gd and DynGd and concrete implementations for built-ins. I don't like it and explored few alternatives (I had some success with approach similar to disjointed impls) and all of them were even worse :I. Doubled Var implementations are bad and I would gladly accept any suggestion for improvement 🫡 .

It ain't that bad tho.

For now OnEditor is enabled ony for very limited set of built-ins (other than object ofc). We will see what are the usecases.

I also found & removed the bug that allowed to use Node-based classes as an #[export] for resource-based classes – the validation was already there in godot-core/src/registry/godot_register_wrappers.rs:30 😅.

@GodotRust
Copy link

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

@Yarwin Yarwin changed the title Add OnEditor<T>, remove impl<T> Export for Gd<T> Add OnEditor<T>, remove impl<T> Export for Gd<T> Feb 18, 2025
@Yarwin Yarwin changed the title Add OnEditor<T>, remove impl<T> Export for Gd<T> Add OnEditor<T>, remove impl<T> Export for Gd<T> and DynGd<T, D> Feb 19, 2025
@Yarwin Yarwin marked this pull request as ready for review February 19, 2025 16:36
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! 👍


I could not find proper blanket implementation that would satisfy all the GodotTypes, so we are stuck with three different impls – blanket implementations for Gd and DynGd and concrete implementations for built-ins. I don't like it and explored few alternatives (I had some success with approach similar to disjointed impls) and all of them were even worse :I. Doubled Var implementations are bad and I would gladly accept any suggestion for improvement 🫡 .

What were the concrete problems you faced, e.g. for implementing

impl<T> Var for OnEditor<T> { ... }

As mentioned in one of the comments, I don't think OnEditor should support general late-init functionality. Spontaneously speaking, I'd keep the overlap in functionality with OnReady to an absolute minimum.

Regarding integration with ready(), can we not just panic if it's not yet initialized at that point? Or what was the reason for allowing manual initialization inside ready(), when OnReady can already do that?

Comment on lines 1089 to 1091
impl<T: Export> Export for Array<T>
where
T: ArrayElement + Export,
T: ArrayElement,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Bounds shouldn't be declared in mixed places.

impl<T: GodotClass> Export for Array<Gd<T>>
where
T: Bounds<Exportable = bounds::Yes>,
Gd<T>: ArrayElement,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this bound necessary?

That trait is implemented for all Gd<T>:

impl<T: GodotClass> ArrayElement for Gd<T> {

where
T: GodotClass + Bounds<Exportable = bounds::Yes>,
D: ?Sized + 'static,
{
fn export_hint() -> PropertyHintInfo {
PropertyHintInfo {
hint_string: get_dyn_property_hint_string::<D>(),
..<Gd<T> as Export>::export_hint()
..Gd::<T>::export_hint()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the explicit trait qualification, it makes code a bit more explicit (and in rare cases, may disambiguate).


#[doc(hidden)]
fn as_node_class() -> Option<ClassName> {
Gd::<T>::as_node_class()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here:

Suggested change
Gd::<T>::as_node_class()
<Gd<T> as Export>::as_node_class()

There are a few more places; but you can also wait in case we find a way to consolidate the trait impls across OnEditor.

Comment on lines 514 to 556
#[doc(hidden)]
#[allow(clippy::derivable_impls)]
impl<T, D> Default for OnEditor<DynGd<T, D>>
where
T: GodotClass,
D: ?Sized + 'static,
{
fn default() -> Self {
OnEditor::null()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding #[doc(hidden)]:

Default is part of the public API -- we should not implement it if we don't want to expose it.

/// #[godot_api]
/// impl INode for MyClass {
/// fn ready(&mut self) {
/// // Field `required_with_default` can be either default value - specified in `#[init]` or value set via the Godot Editor.
/// assert_eq!(self.required_with_default.get_class(), GString::from("Node"));
/// // Will always be valid and must be set via editor – an additional check is being run before ready to make sure that given value can't be null.
/// let some_variant = self.editor_field.get_meta("SomeName");
/// }
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the generated PR output per bot link -- the code needs horizontal scrolling, and some statements are not aligned 🙂

https://godot-rust.github.io/docs/gdext/pr-1051/godot/obj/struct.OnEditor.html

Comment on lines 149 to 161
enum OnEditorState<T> {
// Represents uninitialized, null value.
Null,
// Represents initialized, invalid value.
Uninitialized(T),
Initialized(T),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum OnEditorState<T> {
// Represents uninitialized, null value.
Null,
// Represents initialized, invalid value.
Uninitialized(T),
Initialized(T),
}
enum OnEditorState<T> {
/// Uninitialized null value.
Null,
/// Uninitialized state, but with a value marked as invalid.
Uninitialized(T),
/// Initialized with a value.
Initialized(T),
}

Comment on lines 483 to 484
impl_property_by_godot_convert!(Rect2, oneditor);
impl_property_by_godot_convert!(Rect2i, oneditor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, we should see if this is really necessary, or if a blanket impl is possible.

These individual trait impl are likely not great for compile times, and they also clutter the OnEditor doc page massively:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, much better

image

Comment on lines 295 to 337
let oneditor_panic_inits = {
// Despite its name OnEditor shouldn't panic in the editor for tool classes.
let editor_check = quote! { ::godot::classes::Engine::singleton().is_editor_hint() };

// Inform the user which fields haven't been set, instead of panicking on the very first one. Useful for debugging.
let on_editor_fields_checks = all_fields
.iter()
.filter(|&field| field.is_oneditor)
.map(|field| {
let field = &field.name;
let warning_message =
format! { "godot-rust: OnEditor field {field} hasn't been initialized."};
quote! {
if this.#field.is_invalid() {
::godot::global::godot_warn!(#warning_message);
is_oneditor_properly_initialized = false;
}
}
})
.collect::<Vec<_>>();

if !on_editor_fields_checks.is_empty() {
run_before_ready = true;
quote! {
fn __are_oneditor_fields_initalized(this: &#class_name) -> bool {
if #editor_check {
return true;
}

let mut is_oneditor_properly_initialized: bool = true;
#( #on_editor_fields_checks )*

is_oneditor_properly_initialized
}

if !__are_oneditor_fields_initalized(&self) {
panic!("godot-rust: OnEditor fields must be properly initialized before ready.")
}
}
} else {
TokenStream::new()
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should maybe be its own function...

Copy link
Contributor Author

@Yarwin Yarwin Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved both oneditor_panic_inits and onready_inits to their own functions. Both return TokenStreams and before_ready is being generated only if any of these TokenStreams isn't empty

@Yarwin
Copy link
Contributor Author

Yarwin commented Feb 20, 2025

I could not find proper blanket implementation that would satisfy all the GodotTypes, so we are stuck with three different impls – blanket implementations for Gd and DynGd and concrete implementations for built-ins. I don't like it and explored few alternatives (I had some success with approach similar to disjointed impls) and all of them were even worse :I. Doubled Var implementations are bad and I would gladly accept any suggestion for improvement 🫡 .

What were the concrete problems you faced, e.g. for implementing

impl<T> Var for OnEditor<T> { ... }

I fixed it in meanwhile 🫡.

Long story short, before figuring out the correct bounds for all the types, I've been entangled in conflicting implementations of traits (Gd implements GodotType etc). The correct approach turned out to be fairly simple – I just had to add one extra marker trait into the mix AND tell compiler that T::Via == T – instead of that I've tried to figure out the "correct" bounds on GodotType 😅 .

Negative trait bounds and specialization would come in handy here – albeit they are still work in progress.

For now I moved all blanket implementations to oneditor.rs. The marker trait called BuiltinGodotType is WiP – I'm not sure how to call it (BuiltinGodotType is silly, Gd is godot type) and where to put it.

There is still some clean-up left to do which I'll take care of tomorrow (from initial batch of comments – the Default trait). Implementations of traits on OnEditor has been moved into one file, for now, for better readability before the cleanup.

EDIT: The marker Trait is called DirectExport. All implementations related to exporting Gd<T> and DynGd<T> are in their associated files, while blanket implementations for DirectExport can be found in onready.rs,

@Yarwin Yarwin force-pushed the add-on-editor branch 6 times, most recently from 8bc1c2c to 4540e39 Compare February 22, 2025 16:32
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update! Added some comments.

I'm not really sure about the name "uninit" in OnEditor::unit() + #[init(uninit = ...)]. First, people probably know this from mem::MaybeUninit::uninit(), where it has a totally different meaning (actually uninitialized memory).

Second, it doesn't express that the value is not only "uninitialized", but also a "invalid marker" for value that's used to distinguish valid/invalid state. In programming, sentinel value probably comes close to describe it.

I don't really have a great alternative though, maybe we can brainstorm a bit?

  • Single word: unset, invalid, ...
  • Marker: invalid_marker, uninit_marker
  • Sentinel: uninit_sentinel, ...
  • Descriptive: needs_init
  • Exclusion: except, exclude, excluded, exempt, unless...

(Perfect would be a single word expressing both "needs initialization" and "marks an invalid value", but that's very specific...)

@@ -495,18 +495,69 @@ where
}
}

impl<T, D> Export for DynGd<T, D>
#[allow(clippy::derivable_impls)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please quickly add a comment why this #[allow()] is necessary, and why we don't derive Default here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, Clippy no longer complain about it after adding blanket implementation for OnEditor.

}
}

#[allow(clippy::derivable_impls)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here

fn deref(&self) -> &Self::Target {
match &self.inner {
OnEditorState::Null | OnEditorState::Uninitialized(_) => {
panic!("godot-rust: OnEditor field hasn't been initialized.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for godot-rust: prefix in panics, the handler already prints this when necessary.

Also in other places.

Comment on lines +1111 to +1116
fn as_node_class() -> Option<ClassName> {
PropertyHintInfo::object_as_node_class::<T>()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be #[doc(hidden)] everywhere. Unfortunately rustdoc doesn't seem to be smart enough to pick up the attribute from the trait and apply it to all impls.

Comment on lines 322 to 343
#[doc(hidden)]
pub fn object_as_node_class<T: GodotClass + Bounds<Exportable = bounds::Yes>>(
) -> Option<ClassName> {
T::inherits::<classes::Node>().then(|| T::class_name())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the bounds are nested, or get long enough to require line break, please use a where clause.

Comment on lines 80 to 81
/// Marker trait to identify non-nullable `GodotType`s that can be directly used with `#[export]` and `#[var]`.
pub trait DirectExport {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to address the "too many impls" problem, thanks! 👍

I'm not sure if the term "direct" is very precise. What about BuiltinExport -- in practice, all types for which the trait is implemented are builtins, no?

An alternative would also be to check this via Bounds associated type -- like the Exportable type here, it could be something like BuiltinExportable or so.

/// True if *either* `T: Inherits<Node>` *or* `T: Inherits<Resource>` is fulfilled.
///
/// Enables `#[export]` for those classes.
#[doc(hidden)]
type Exportable: Exportable;

These are all just ideas, I'm fine with getting the basics sorted in this PR and then refine later 🙂

Copy link
Contributor Author

@Yarwin Yarwin Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it via Bounds and it seems to work – therefore DirectExport is now a part of the GodotType as an associated type BuiltinExportable: Exportable;

The question is if we want to move declaration of Exportable and bounds::Yes/No, for now on I left it in obj/bounds.rs 🤷..

Earlier on I couldn't get it working 🤔 (probably because I haven't specified both OnEditor<T>: GodotConvert<Via = T> and T: GodotConvert<Via = T> in required bounds).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to use an associated type inside Bounds, not GodotType. But I missed that Bounds doesn't support non-class types 🤦‍♂️ But I don't think GodotType is a good idea, this is a very core type that almost everything must implement, so adding more requirements there has far-reaching consequences.

Probably the original proposal of yours was indeed better. There are some marker traits in godot::meta already; we could possibly add BuiltinExportable there?

Also, where does Variant stand in this? Like object, it has a null state (Variant::nil()), should OnEditor expect that it's non-nil as well?

Comment on lines 267 to 277
if let Some(first) = onready_fields.next() {
quote! {
{
let base = <Self as godot::obj::WithBaseField>::to_gd(self).upcast();
#first
#( #onready_fields )*
}
}
} else {
TokenStream::new()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is only the first field read here?

Or what's the point of mapping/filtering the whole slice and then discard all but one element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I blindly copied&pasted this code to new function 😅 – frankly I had little issues as well, since everything else is using collect and Vec 🤔. It just checks if iterator is empty by fetching the very first element.

I rewrote it to use collect and Vec instead.

Comment on lines 304 to 310
if #editor_check {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments about these semantics -- they are not obvious to someone reading the code in the future.

Comment on lines 308 to 311
let mut is_oneditor_properly_initialized: bool = true;
#( #on_editor_fields_checks )*

is_oneditor_properly_initialized
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here -- "properly" sounds vague and sloppy; if there isn't a more precise name, at least a comment should be used to clarify.


#[derive(GodotClass)]
#[class(init)]
struct RefcDynGdExporter {
struct RefcHasDynGdVar {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct RefcHasDynGdVar {
struct RefcDynGdVarDeclarer {

maybe? 😅

@Yarwin Yarwin force-pushed the add-on-editor branch 4 times, most recently from 6b5fdad to 66646bb Compare February 25, 2025 15:52
@Yarwin
Copy link
Contributor Author

Yarwin commented Feb 25, 2025

I cleaned PR, here is short, and I hope, non-chaotic, coherent description.

What problem this PR is aiming to solve?

  • Cases in which editor sets non-nullable properties to null.
  • Memory leaks – due to requirement to create default value – in cases when non-nullable properties were being used as an #[export].
  • Ergonomic improvements over Option<T> for such values.

Modus operandi of this PR has been documented in newly created BuiltinExport marker trait:

/// Marker trait to identify `GodotType`s that can be directly used with an `#[export]`.
///
/// Implemented pretty much for all the [`GodotTypes`][GodotType] that are not [`GodotClass`].
/// Provides a few blanket implementations and, by itself, has no implications
/// for the [`Var`] or [`Export`] traits.
///
/// Some Godot Types which are inherently non-nullable (e.g., `Gd<T>`),
/// might have their value set to null by the editor. Additionally, Godot must generate
/// initial, default value for such properties, causing memory leaks.
///
/// Non-algebraic types that don't implement `BuiltinExport` shouldn't be used directly as an `#[export]`
/// and must be handled using associated algebraic types, such as:
/// * [`Option<T>`], which represents optional value that can be null when used.
/// * [`OnEditor<T>`][crate::obj::OnEditor], which represents value that must not be null when used.
pub trait BuiltinExport {}

How does this PR solves aforementioned issues?

  • Introducing new algebraic type – OnEditor<T> . OnEditor<T> either has valid value to which it defers, or holds invalid value (null or specified concrete value) – and panics when trying to access it.
  • OnEditor supports two workflows – setting given values via the Editor or doing so by some kind of factory/builder pattern. Initialization logic is set in a way that makes it annoying to use outside of specific creation context. i.e – OnEditor fields MUST be set when object is being put into use.
  • Removing Export and Var implementations for Gd<T> and DynGd<T> (Var due to tool implications).
  • Types that are non-nullable – that is, ones not implementing BuiltinExport trait – but must be nullable in editor must be handled via associated algebraic types

Docs has been written to make purpose and use of these features clear.

What are extra features implemented in this PR?

  • When used on nodes, OnEditor<T> runs before-ready check and warns user what types are invalid and then panics if any OnEditor<T> field is invalid.
  • OnEditor<T> can be used with types that implement BuiltinExport. OnEditor<T> allows one to mark some concrete values as invalid and panics if it haven't been changed.
  • DynGd<T, D> can be used as an property only for T which isn't concrete user-defined rust class. DynGd<T, D> provides shared functionality D among objects inheriting T – shared functionality for one concrete type doesn't make much sense.

What are some open issues?

  • Idk if removing Var for Gd<T> and DynGd<T, D> is good idea, but in general one almost always wanted to use it wrapped in Option. Removing Var also makes impls a bit messier.
  • Removing #[export] DynGd<T, D> for T being rust class might not be the best choice 🤔. I left #[var] intact.
  • Marker trait is just a marker and doesn't have anything to do with either Var and Export traits 🤔. It is purely informational & allows to auto-generate few blanket implementations – IMO it should stay this way (name suggest that it is used to mark built-in; both Option<...> and OnEditor<...> are non-builtin types)

What still needs to be done?

  • I need to give this PR another good test run in real-world environment

@Yarwin Yarwin force-pushed the add-on-editor branch 4 times, most recently from 48162fa to 608c3f6 Compare February 26, 2025 08:20
…nGd` and `Gd`

- Add `OnEditor<T>` - a wrapper that allows to export non-nullable types which must be nullable in the Editor.
- Remove Export and Var implementations for `DynGd<T, D>` and `Gd<T>` - it should be provided by algebraic types instead (OnEditor and Option).
- Add marker trait `BuiltinExport` which informs if given type can be safely and conveniently used in a `#[export]` directly.
- Create implementations for `OnEditor<Gd<T>>` and `OnEditor<DynGd<D, T>>`.
- Create proper blanket implementation for Godot Types other than Gd<T>
- Inform user about available use cases for `OnEditor<T>` in associated docs
- Move Gd<T>::export_info to PropertyHintInfo::export_gd.
- Add before_ready check for OnEditor fields to panic and warn user in case if values haven't been set.
- FIX: `Option<Gd<T>>` and `OnEditor<Gd<T>>` can no longer be used as an Export for Resource-based classes for `T` Inheriting Node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Requires SemVer bump c: core Core components c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OnEditor<T>/Required<T>for #[export] similar to OnReady<T>
3 participants