bodoni / svg

Composer and parser for SVG
Other
302 stars 44 forks source link

Update Data Element within a struct #62

Closed MhhhxX closed 1 year ago

MhhhxX commented 1 year ago

I'm trying to implement a function inside a struct which updates a Vector of Data elements.

struct Svg {
    data: Vec<Data>,
}

impl Svg {
    fn new() -> Self {
        Self {
            data: {
                let mut vec = Vec::<Data>::with_capacity(10);
                for _ in 0..10 {
                    vec.push(Data::new().move_to((0, 0)));
                }
                vec
            },
        }
    }

    fn update(&mut self) {
        self.data[0].line_to((4,9));
    }
}

When I compile this code I get the following error: move occurs because value has type "svg::node::element::path::Data", which does not implement the "Copy" trait. I'm new to rust but I guess the move occurs because the function line_to takes mut self as a parameter which moves the ownership. Is there a way to make my code compile?

I need this code because I run a simulation which produces data every time step and I want to draw an svg picture from it. I therefore try to collect the data from the simulation inside a Data elements to draw Paths from them. The example above is only a prototype for that. It's not for practical use yet.

IvanUkhov commented 1 year ago

Hello, I see you create a vector of a specific capacity. It does not automatically initialize the vector. The length is zero where you try to iterate. Instead, you probably want to create data inside the loop and then push into the vector.

let mut vector = Vec::with_capacity(10);
for _ in 0..10 {
    vector.push(Data::new().move_to((0, 0)));
}
MhhhxX commented 1 year ago

I didn't know that. Thank you. I change my code according your suggestion. But I still have this error in the fn update() function when I call the line_to() function.

IvanUkhov commented 1 year ago

Yes, from that position it can be tricky, as it moves the object. If you want to keep this architecture, you could use std::mem::swap to swap the element in the array with an empty Data, mutate, and then swap back via the same function. Or better yet, via std::mem::replace. You could also store something that can be moved out. For instance, you could store Vec<Option<Data>> and then do vector[0].take() to get the data, leaving None behind, and then put back with Some.

MhhhxX commented 1 year ago

Thank you for your help. That sounds indeed a bit tricky. May I ask why you choose to program your API like that?

I think I change the architecture and collect the data first and then, when the simulation ends I create the svg from the data.

IvanUkhov commented 1 year ago

It is fair criticism. The package was written with some specific use cases in mind, which made some other less convenient. The operation chaining was desired, for instance, which, however, made after-the-fact modifications more difficult. If you have any proposals, I would be happy to discuss.

MhhhxX commented 1 year ago

I think I have to go deeper into the source code to give good proposals. But my first though was changing the parameter mut self to &mut self. Then the line_to function doesn't take the ownership. But would that break something else?

IvanUkhov commented 1 year ago

Exactly. And the whole design is around this possibility of creating and chaining in one line.

I have just realized that, unlike other object where there is a duality between add (consuming) and append (non-consuming), Data was lacking the latter. I have just added a non-consuming version:

https://github.com/bodoni/svg/blob/main/src/node/element/path/data.rs#L295-L300

It will be a bit more verbose, but it should do the trick in your update function.

MhhhxX commented 1 year ago

Yes, thank you! Exactly what I searched for. I'll check that out 👍

To add my specific use case without breaking the chaining functionality I only can see one way to archive that. Adding second variants to each function like you do it with add and append. For move_to we could introduce the non consuming variant move_to_nc for example.