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

Deep copies taking into account Rust state (unlike Node::duplicate()) #695

Open
Riizade opened this issue May 4, 2024 · 8 comments
Open
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@Riizade
Copy link

Riizade commented May 4, 2024

Taking the docs as an example,

#[derive(GodotClass)]
#[derive(GodotClass)]
#[class(base=Node3D)]
struct Monster {
    name: String,
    hitpoints: i32,
    base: Base<Node3D>,
}

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

@lilizoey lilizoey added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels May 4, 2024
@lilizoey
Copy link
Member

lilizoey commented May 4, 2024

I am skeptical this can be done in general, since it'd require somehow overriding the duplicate implementation for each class. This might require upstream Godot to support such a use-case by providing a virtual duplicate method for us to override.

@Riizade
Copy link
Author

Riizade commented May 4, 2024

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 #[var] at the moment.

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.

@Bromeon
Copy link
Member

Bromeon commented May 4, 2024

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 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.


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.

If I understand right, we need a method that:

  • calls Clone::clone for each field except Base<T>
  • calls duplicate on the Base<T>

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 Clone or field-wise cloning isn't semantically meaningful.

Also, Node::duplicate() is only supported for Node derivates, and takes flags for customization. I could imagine that people would want deep copying for other types, e.g. RefCounted.

I'll reword the title slightly, and we can gladly use this issue for design discussions 🙂

@Bromeon Bromeon changed the title .duplicate() does not function as expected with no alternative for the functionality Deep copies taking into account Rust state (unlike Node::duplicate()) May 4, 2024
@lilizoey
Copy link
Member

lilizoey commented May 4, 2024

additionally it should recursively call duplicate on its children if it is a node i think?

@Riizade
Copy link
Author

Riizade commented May 6, 2024

I think the piece missing from @Bromeon's post is, as @lilizoey said, that .duplicate() is recursive, i.e., it copies the node and all of its children, not just the node itself.

You can define your own method to copy an instance, although admittedly it's not that ergonomic.

I've been thinking about this, and I think you could write a trait Duplicable that has duplicate_self(), then you'd have a duplicate2() method that crawls the parent node and its subtree calling duplicate_self() for each node, where duplicate_self() calls .duplicate() for itself (with the appropriate flags to copy only itself, not copy the node's subtree recursively).

You'd then have an impl<T> Duplicable for Gd<T> where T: Inherits<Node> + GodotClass where duplicate_self() just duplicates the node using Godot's API (again, non-recursively), since there are no members Godot is unaware of.

This is writeable as a proc macro for gdext-defined node types, which I think is worth placing into the gdext library as a part of #[derive(GodotClass).

But as lilizoey said above, the difficult piece as a user of the library is overriding duplicate() such that this happens as expected for that method, rather than implementing a new duplicate2() or similar for the functionality.

In any case

I think this is overly dramatic.

Point taken, and it's definitely worth trying to implement this in gdext without contacting the Godot team, and only put in a feature request if it seems unworkable. I'm not familiar with the GDextension API, so I don't know if overriding of this nature is possible or not.

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.

@Bromeon
Copy link
Member

Bromeon commented May 6, 2024

Thanks for the elaboration! I think what adds some complexity for duplicate() is that unlike clone(), the user may want different semantics depending on needs. There are multiple dimensions to this:

  1. Node::duplicate() itself takes DuplicateFlags which customize what is duplicated.
  2. The node's children may be deep-copied, but this may still reuse some shared resources (materials, shapes, etc). I think that's fine though, and up to the user to detangle if needed.
  3. Since not all classes inherit from Node, a duplicate() function will not always be available, but it could still make sense to allow e.g. copying of RefCounted ones.

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 BaseCopier a bit overengineered... The point is that the user has control over how the copy happens.
But it could also just be something very general like

let new_base = copier.run(|original: &Node| -> Gd<Node> {
    // user can themselves create a new Gd, e.g. with duplicate() or manually
})

@Riizade
Copy link
Author

Riizade commented May 6, 2024

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),
    ...
}

@Bromeon
Copy link
Member

Bromeon commented May 6, 2024

Interesting idea, although I'd probably start without that first, and see what kind of workflows/patterns emerge 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

3 participants