-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Deep copies taking into account Rust state (unlike Node::duplicate()
)
#695
Comments
I am skeptical this can be done in general, since it'd require somehow overriding the |
I think it's worth bringing up to the Godot team, since this seems like a pretty damaging limitation of the GDextension API if this can't be done. I think the intended route is that all of the properties of a Node are exposed to the Godot engine though; which is fine by me. gdext seems quite limited in this regard; there's a derive macro but it only works for newtypes and enums with no fields if I understand correctly. It seems quite difficult to support an arbitrary struct/enum with Is there any possibility of using serde to serialize to, say, a binary blob on the Godot node? Even if it can't be meaningfully viewed or edited in the engine editor, it solves cases like duplicate() not knowing about the existence of certain properties. |
I think this is overly dramatic. You can define your own method to copy an instance, although admittedly it's not that ergonomic. Once we support traits (#426 or #631), this could also be done generically/polymorphically. But if you think this needs a change in Godot engine, please don't hesitate to open a discussion in godot-proposals. Maybe check if there aren't similar proposals already.
If I understand right, we need a method that:
and thus returns a new, independent copy. Does that make sense? Additionally, we'd need to provide a way for the user to customize this, in case individual fields don't implement Also, I'll reword the title slightly, and we can gladly use this issue for design discussions 🙂 |
Node::duplicate()
)
additionally it should recursively call duplicate on its children if it is a node i think? |
I think the piece missing from @Bromeon's post is, as @lilizoey said, that
I've been thinking about this, and I think you could write a trait You'd then have an This is writeable as a proc macro for gdext-defined node types, which I think is worth placing into the But as lilizoey said above, the difficult piece as a user of the library is overriding In any case
Point taken, and it's definitely worth trying to implement this in Anyway, this is not a show-stopper for me; I have a different workaround for my use case that does not work generically, and I suspect many use cases will have similar workarounds available. |
Thanks for the elaboration! I think what adds some complexity for
Without a lot of deep thought about the design, something I could imagine would be: #[derive(GodotClass)]
#[class(init, duplicate)] // <- (1) implements a common default
struct MyClass {
base: Base<Node>,
health: i32,
}
#[godot_api]
impl INode for MyClass {
// (2) alternatively, implement manually
fn on_duplicate(&self, copier: BaseCopier<Node>) -> Self {
// copier offers configuration on how to copy the base
let new_base = copier
.duplicate_flags(...)
.other_config(...)
.build();
Self {
base: new_base ,
health: self.health,
}
}
} Maybe let new_base = copier.run(|original: &Node| -> Gd<Node> {
// user can themselves create a new Gd, e.g. with duplicate() or manually
}) |
I think that looks great! I'd just like to add that for the derived version, I think it's possible to add a lot of flexibility with attributes. e.g.: #[derive(GodotClass)]
#[class(init, duplicate, default_duplicate=DupeStrategy::Clone)] // specifies the duplication strategy for fields which do not have an explicit attribute
struct MyClass {
base: Base<Node>,
#[duplicate(DupeStrategy::Clone)] // clones this field on duplicate
health: i32,
#[duplicate(DupeStrategy::Ignore)] // ignores this field on duplicate (resulting node's property will have whatever is left at the end of init())
muffins: u32,
#[duplicate(DupeStrategy::Fn(duplicate_string))] // uses the return value of the function
cupcakes: String,
#[duplicate(DupeStrategy::GenerateFromNode(gen_str)] // same as above, but passes in the entire node rather than just the original property value
croissants: Vec<String>,
}
fn duplicate_string(original: String) -> String {
format!("{original}-duplicated")
}
fn gen_str(node: NodeType) -> Vec<String> {
...
} which implies an enum in the proc macro pub enum DupeStrategy {
Clone,
Ignore,
Fn(fn(T) -> T),
GenerateFromNode(fn(NodeType) -> T),
...
} |
Interesting idea, although I'd probably start without that first, and see what kind of workflows/patterns emerge 🙂 |
If there's still interest in pursuing this, I need to know how people have addressed this problem until now. "Duplicate for Rust classes" sounds fancy, but it can mean lots of things:
I'm really not sure if we can find a generic solution that supports all the above cases. As such, I need to see how users have addressed this problem with specialized functions first. Then we can get a feel if/how it's possible to generalize... but I want to avoid that we spend huge effort on a mega-solution when everyone just builds their custom impl anyway. Also, there are several limitations without engine support: the function to copy won't be accessible in base classes, |
Taking the docs as an example,
Calling .duplicate() on an instance will correctly copy all the stuff that Godot knows about, but name and hitpoints will be their .init() values
It makes sense that it works this way to a certain degree, since duplicate() is a Godot engine thing, so .duplicate() just calls the Godot function without doing anything special for the gdext portions, but it was surprising to me, and may be to others
There should be a way to fully clone a node and its children including their Rust properties, without requiring that each Rust property is
#[var]
-compatible.original Discord thread: https://discord.com/channels/723850269347283004/1236100168722681927
The text was updated successfully, but these errors were encountered: