mitchmindtree / rose_tree-rs

An implementation of the "rose tree" data structure for Rust.
Apache License 2.0
24 stars 6 forks source link

Added RoseTree.graph_mut() to allow mutable access to underlying petgraph. #1

Closed zmoshansky closed 8 years ago

zmoshansky commented 8 years ago

Would you consider adding this method so that one can obtain mutable access to the underlying petgraph? AFAIK, it's the only easy way* to use the algorithms in petgraph to do traversal while maintaining mutable access to the underlying nodes (without consuming the RoseTree via into_graph).

The context I'm using it in is a scene graph backed by rose_tree, so I'm mutably traversing the graph to do layout, and collision detection; But, immutably traversing it for rendering.

*The alternative being to make nodes of type RefCell<Node>?

    let mut graph = tree.graph_mut();

    let mut dfs = petgraph::Dfs::new(graph, petgraph::graph::NodeIndex::new(ROOT));
    while let Some(node_index) = dfs.next(graph) {
        let node = graph.node_weight_mut(node_index).unwrap();
       ...........
mitchmindtree commented 8 years ago

Hey @zmoshansky, thanks for the PR :)

Hmmm the main issue with this is that if we provide a mutable reference to the inner graph directly, we can no longer guarantee that the RoseTree is really a tree, which is an assumption that a few of the methods depend on.

Now that I think of it, I don't think that example should require accessing the graph mutably at all? Dfs doesn't require mutable access to the Graph, and you should be able to use those node_indexs with the RoseTree itself. i.e something like this should work fine:

let mut dfs = petgraph::Dfs::new(tree.graph(), petgraph::graph::NodeIndex::new(ROOT));
while let Some(node_index) = dfs.next(tree.graph()) {
    let node = &mut tree[node_index];
    ...........

Let me know how this goes!

zmoshansky commented 8 years ago

Good point about enforcing the tree invariant, and you're right. Thanks for your help! You wouldn't believe how may variations I tried unsuccessfully, including weeping at the borrow checker ;) Of course now that I see it working, it makes sense. But curse that extra .graph() lol.

Works

    let node = &mut tree[node_index];
    let node = tree.node_weight_mut(node_index).unwrap();

Doesn't Work....and all the permutations I tried with different bindings :P

    let node = &mut tree.graph().node_weight_mut(node_index).unwrap();
error: cannot borrow immutable borrowed content as mutable
let node = &mut tree.graph().node_weight_mut(node_index).unwrap();