Added instantiate_as for PackedScene #299

Brocktho · 2023-06-05T21:04:23Z

I added PackedSceneExt as a trait for the PackedScene to impl, made the impl and added usage of instantiate_as to the example dodge the creeps.

I commented out areas in the dodge-the-creeps file.

Bromeon

Thanks!

Is GenEditState being explicitly provided a common occurrence? I think the idea of instantiate_as is to cover the common case in an easy way, and adding significant type overhead for the 2nd parameter seems to work against that.

I'd keep it simple and just support the call without GenEditState. If this becomes required, we can always change things later.

Please also consider the contribution guidelines, especially 1 commit per logical change 🙂

GodotRust · 2023-06-05T23:08:56Z

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

Brocktho · 2023-06-06T19:04:45Z

Would you like me to just turn GenEditState into a no arg function and have it add the GenEditState::GEN_EDIT_STATE_DISABLED. Then if the user wants to have more control they manage the instantiate themselves with the base PackedScene? I'm definitely not opposed to it after using the current method more, just initially posted it trying out the tuple approach to see how it felt.

Bromeon · 2023-06-06T19:12:41Z

Exactly; I think an API like

    fn try_instantiate_as<T>(&self) -> Option<Gd<T>>
    where
        T: Inherits<Node>;

    fn instantiate_as<T>(&self) -> Gd<T>
    where
        T: Inherits<Node>;

is enough.

Please document the methods 🙂

Brocktho · 2023-06-06T19:39:03Z

So you made a note about multiple commits, what should I do to make said changes? Most of my extra commits on here were mistakes of not fully pushed code, also now I'm assuming is safe to push an extra commit?

Bromeon · 2023-06-06T20:20:34Z

Check the link I posted, there is a short explanation in the guidelines. If you haven't used those git commands before, https://git-scm.com/docs is a good resource.

Brocktho · 2023-06-06T21:14:18Z

what a mess...
working on it lol

Brocktho · 2023-06-06T21:45:20Z

My lord, that was painful. Rebasing is unpleasant.

Bromeon

Thanks for the update! Left a few comments.

With one commit, it's quite easy to do changes: just use git commit --amend.

Bromeon · 2023-06-06T21:52:29Z

examples/dodge-the-creeps/rust/src/main_scene.rs

@@ -78,7 +77,7 @@ impl Main {
            .base
            .get_node_as::<PathFollow2D>("MobPath/MobSpawnLocation");

-        let mut mob_scene: Gd<RigidBody2D> = instantiate_scene(&self.mob_scene);
+        let mut mob_scene: Gd<RigidBody2D> = self.mob_scene.instantiate_as();


Maybe use this notation, as it reads more idiomatically "instantiate as RigidBody2D":

Suggested change

let mut mob_scene: Gd<RigidBody2D> = self.mob_scene.instantiate_as();

let mut mob_scene = self.mob_scene.instantiate_as::<RigidBody2D>();

Bromeon · 2023-06-06T21:54:36Z

godot-core/src/engine.rs

+
+/// Extension trait for convenience functions on `PackedScene`
+pub trait PackedSceneExt {
+    /// Instantiates the scene as type `T`, panicking if not found or bad type.


Suggested change

/// Instantiates the scene as type `T`, panicking if not found or bad type.

/// ⚠️ Instantiates the scene as type `T`, panicking if not found or bad type.

The emoji is for IDE inline docs, a convention we use for Gd::cast(), Gd::from_instance_id(), etc.

Bromeon · 2023-06-06T21:55:48Z

godot-core/src/engine.rs

+    /// If the scene is not type `T` or inherited.
+    fn instantiate_as<T>(&self) -> Gd<T>
+    where
+        T: GodotClass + Inherits<Node>,


As in my previous suggestion, GodotClass is not necessary, since it's a bound implied by Inherits.

Bromeon · 2023-06-06T21:57:11Z

godot-core/src/engine.rs

+    where
+        T: GodotClass + Inherits<Node>,
+    {
+        self.try_instantiate_as::<T>().unwrap()


Instead of unwrap(), could you add a descriptive error message taking into account the variables?

For prior art, see

gdext/godot-core/src/obj/gd.rs

Lines 316 to 331 in 7573721

/// ⚠️ **Downcast:** convert into a smart pointer to a derived class. Panics on error.

///

/// # Panics

/// If the class' dynamic type is not `Derived` or one of its subclasses. Use [`Self::try_cast()`] if you want to check the result.

pub fn cast<Derived>(self) -> Gd<Derived>

where

Derived: GodotClass + Inherits<T>,

{

self.owned_cast().unwrap_or_else(|from_obj| {

panic!(

"downcast from {from} to {to} failed; instance {from_obj:?}",

from = T::CLASS_NAME,

to = Derived::CLASS_NAME,

)

})

}

Bromeon · 2023-06-06T21:58:19Z

godot-core/src/engine.rs

+        self.instantiate(GenEditState::GEN_EDIT_STATE_DISABLED)
+            .map(|gd| gd.cast::<T>())


This panics if cast() fails, violating the method contract. Use try_cast() instead.

Bromeon · 2023-06-06T22:05:35Z

Worth noting, now the extension trait is implemented on PackedScene (and not Gd<PackedScene>), but as mentioned on Discord, I think it's fine for now.

We may want to revisit this topic later down the line, with load, get_node_as, and possibly more extension functions like that.

Bromeon · 2023-06-07T08:00:29Z

godot-core/src/engine.rs

+        self.instantiate(GenEditState::GEN_EDIT_STATE_DISABLED)
+            .map(|gd| {
+                gd.try_cast::<T>().unwrap_or_else(|| {
+                    panic!("Failed to cast {self:?} into {to}", to = T::CLASS_NAME,)
+                })
+            })


You still panic here. The idea of try_ functions is to not panic, but propagate the error (or in this case, None).

Bromeon · 2023-06-07T08:03:47Z

godot-core/src/engine.rs

+        T: Inherits<Node>,
+    {
+        self.try_instantiate_as::<T>()
+            .unwrap_or_else(|| panic!("Failed to instantiate {to}", to = T::CLASS_NAME,))


trailing comma

Updates based on provided comments. panic with messages of failed instantiation into T Understood that I needed to return an option, just lost the plot because the map statement was giving a nested Option, and_then is what I was looking for. Remove trailing comma

Brocktho · 2023-06-07T09:41:54Z

Yikes, should be properly addressed now. I understood I needed to pass back an option, just lost the plot because of the map statement giving a nested option, and_then is what I was looking for.

Bromeon

Thanks!

bors r+

bors · 2023-06-07T13:28:20Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bromeon added c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library labels Jun 5, 2023

Bromeon reviewed Jun 5, 2023

View reviewed changes

Brocktho force-pushed the master branch from ea09e11 to 7537f05 Compare June 6, 2023 21:44

Bromeon reviewed Jun 6, 2023

View reviewed changes

Brocktho force-pushed the master branch from 7537f05 to 24d5817 Compare June 6, 2023 22:25

Bromeon reviewed Jun 7, 2023

View reviewed changes

Brocktho force-pushed the master branch from 24d5817 to 31b9146 Compare June 7, 2023 09:38

Brocktho force-pushed the master branch from 31b9146 to c30cd23 Compare June 7, 2023 09:40

Bromeon reviewed Jun 7, 2023

View reviewed changes

bors bot merged commit 21cf712 into godot-rust:master Jun 7, 2023

Brocktho mentioned this pull request Jun 8, 2023

Ergonomics of instantiating PackedScene #296

Closed

Bromeon mentioned this pull request Jun 13, 2023

Utilities to work with the node tree #9

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added instantiate_as for PackedScene #299

Added instantiate_as for PackedScene #299

Brocktho commented Jun 5, 2023

Bromeon left a comment •

edited

Loading

GodotRust commented Jun 5, 2023

Brocktho commented Jun 6, 2023

Bromeon commented Jun 6, 2023

Brocktho commented Jun 6, 2023 •

edited

Loading

Bromeon commented Jun 6, 2023

Brocktho commented Jun 6, 2023

Brocktho commented Jun 6, 2023

Bromeon left a comment

Bromeon Jun 6, 2023

Bromeon Jun 6, 2023

Bromeon Jun 6, 2023

Bromeon Jun 6, 2023

Bromeon Jun 6, 2023

Bromeon commented Jun 6, 2023

Bromeon Jun 7, 2023

Bromeon Jun 7, 2023

Brocktho commented Jun 7, 2023

Bromeon left a comment

bors bot commented Jun 7, 2023

	let mut mob_scene: Gd<RigidBody2D> = self.mob_scene.instantiate_as();
	let mut mob_scene = self.mob_scene.instantiate_as::<RigidBody2D>();

	/// Instantiates the scene as type `T`, panicking if not found or bad type.
	/// ⚠️ Instantiates the scene as type `T`, panicking if not found or bad type.

	/// ⚠️ Downcast: convert into a smart pointer to a derived class. Panics on error.
	///
	/// # Panics
	/// If the class' dynamic type is not `Derived` or one of its subclasses. Use [`Self::try_cast()`] if you want to check the result.
	pub fn cast<Derived>(self) -> Gd<Derived>
	where
	Derived: GodotClass + Inherits<T>,
	{
	self.owned_cast().unwrap_or_else(\|from_obj\| {
	panic!(
	"downcast from {from} to {to} failed; instance {from_obj:?}",
	from = T::CLASS_NAME,
	to = Derived::CLASS_NAME,
	)
	})
	}

		self.instantiate(GenEditState::GEN_EDIT_STATE_DISABLED)
		.map(\|gd\| gd.cast::<T>())

Added instantiate_as for PackedScene #299

Added instantiate_as for PackedScene #299

Conversation

Brocktho commented Jun 5, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

GodotRust commented Jun 5, 2023

Brocktho commented Jun 6, 2023

Bromeon commented Jun 6, 2023

Brocktho commented Jun 6, 2023 • edited Loading

Bromeon commented Jun 6, 2023

Brocktho commented Jun 6, 2023

Brocktho commented Jun 6, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Jun 6, 2023

Choose a reason for hiding this comment

Bromeon Jun 6, 2023

Choose a reason for hiding this comment

Bromeon Jun 6, 2023

Choose a reason for hiding this comment

Bromeon Jun 6, 2023

Choose a reason for hiding this comment

Bromeon Jun 6, 2023

Choose a reason for hiding this comment

Bromeon commented Jun 6, 2023

Bromeon Jun 7, 2023

Choose a reason for hiding this comment

Bromeon Jun 7, 2023

Choose a reason for hiding this comment

Brocktho commented Jun 7, 2023

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Jun 7, 2023

Bromeon left a comment •

edited

Loading

Brocktho commented Jun 6, 2023 •

edited

Loading