diff --git a/README.md b/README.md index 2784fe8..8abee6d 100644 --- a/README.md +++ b/README.md @@ -69,8 +69,6 @@ related to Rust. - Fix `ramhorn_branch`. - Once I've fixed that, see about a refactor that respects the same model, but involves much less ceremony and boilerplate. -- Figure out the crash bug in `vec_indexed!` if I put a Vertex - *after* an Arg. See my DEBUG notes in `connect()`. - Look at performance. - Start at `to_mesh_iter()`. The cost of small appends/connects seems to be killing performance. diff --git a/src/examples.rs b/src/examples.rs index 23093e6..1f244ce 100644 --- a/src/examples.rs +++ b/src/examples.rs @@ -789,14 +789,14 @@ pub fn ramhorn() -> Rule<()> { let (a0, a1, a2, a3, s4, s5, s6, s7); let seed = vec_indexed![ - @s4 VertexUnion::Vertex(vertex(-0.5, -0.5, 1.0)), - @s5 VertexUnion::Vertex(vertex(-0.5, 0.5, 1.0)), - @s6 VertexUnion::Vertex(vertex( 0.5, 0.5, 1.0)), - @s7 VertexUnion::Vertex(vertex( 0.5, -0.5, 1.0)), @a0 VertexUnion::Arg(0), @a1 VertexUnion::Arg(1), @a2 VertexUnion::Arg(2), @a3 VertexUnion::Arg(3), + @s4 VertexUnion::Vertex(vertex(-0.5, -0.5, 1.0)), + @s5 VertexUnion::Vertex(vertex(-0.5, 0.5, 1.0)), + @s6 VertexUnion::Vertex(vertex( 0.5, 0.5, 1.0)), + @s7 VertexUnion::Vertex(vertex( 0.5, -0.5, 1.0)), ]; let geom = MeshFunc { verts: seed, @@ -812,7 +812,13 @@ pub fn ramhorn() -> Rule<()> { ], }; let final_geom = MeshFunc { - verts: vert_args(0..4), + verts: vec![ + VertexUnion::Arg(s4), + VertexUnion::Arg(s5), + VertexUnion::Arg(s6), + VertexUnion::Arg(s7), + ], + // TODO: Factor out this repetition faces: vec![ 0, 2, 1, 0, 3, 2, diff --git a/src/mesh.rs b/src/mesh.rs index 897e884..add220d 100644 --- a/src/mesh.rs +++ b/src/mesh.rs @@ -155,10 +155,10 @@ impl MeshFunc { /// Treat this mesh as a 'parent' mesh to connect with any number /// of 'child' meshes, all of them paired with their respective /// vertex argument values (i.e. `arg_vals` from `Child`). - /// This returns a tuple of (new mesh, new `arg_vals`), where - /// `arg_vals[i]` gives the remapped value of `children[i].arg_vals` - /// which accounts for where the referenced vertices were moved to. - /// (TODO: That definition is wrong; explain it right.) + /// + /// This returns a tuple of (new `MeshFunc`, new `arg_vals`), where + /// `arg_vals[i]` is the new index of `self.verts[i]` in the + /// returned `MeshFunc`. pub fn connect(&self, children: T) -> (MeshFunc, Vec>) where U: Borrow, T: IntoIterator)> @@ -170,78 +170,71 @@ impl MeshFunc { let mut verts: Vec = self.verts.clone(); let mut faces = self.faces.clone(); - let mut offsets: Vec> = vec![]; + let mut remaps: Vec> = vec![]; - println!("DEBUG: start verts={:?}", verts); - - for (child_, mapping) in children { + for (child_, arg_vals) in children { let child = child_.borrow(); - // offset corresponds to the position in 'verts' at - // which we're appending everything in 'child.verts' - - // thus, the offset we shift all indices in 'children' by. + // 'offset' corresponds to the position in 'verts' at + // which we begin appending everything in 'child.verts'. let offset = verts.len(); + // 'remap[i]' - if 'child.verts[i]' is a Vertex, not an Arg - + // will contain the index in 'verts' that this vertex was + // copied to. (This is needed because below we copy only + // the Vertex elements, not the Arg ones.) let mut remap = vec![0; child.verts.len()]; let mut j = 0; - // Concrete vertices in 'child.verts' need to be copied: + // Like mentioned, copy just the Vertex in 'child.verts': verts.extend(child.verts.iter().enumerate().filter_map(|(i,v)| { match v { VertexUnion::Vertex(_) => { - println!("placing child vert {} at {}", i, offset + j); - remap[i] = j; + // TODO: A less-side-effectful way? + remap[i] = offset + j; j += 1; Some(v.clone()) }, VertexUnion::Arg(_) => None, } })); + // So, note that: + // 1st Vertex in 'child.verts' went to 'verts[offset + 0]'. + // 2nd Vertex in 'child.verts' went to 'verts[offset + 1]'. + // 3rd Vertex in 'child.verts' went to 'verts[offset + 2]'. + // and so forth. + // Since this skips Arg elements, we used 'remap' to + // store the mapping: 'child.verts[i]' was copied to + // 'verts[remap[i]].' - // DEBUG: - // The value of 'n' is an index into the array of vertices. - // That includes both Vertex and Arg. - // The bug is that when 'n' refers to a Vertex, we treat it as - // an index into just the Vertex parts of this (note that we - // copy only those, not the Arg). + // We then use 'remap' below to update the indices in + // 'child.faces' to vertices in the returned mesh. // - // If all Vertex appear *before* all Arg in the array, then - // these are equivalent (there are m Vertex elements, and they - // occupy positions 0..m, and 'n' will also be 0..m... and the - // i'th index is always the i'th Vertex) - and the bug doesn't - // manifest. - // As soon as the i'th index is *not* the i'th Vertex, i.e. there - // is an Arg before it, the bug appears. - - println!("DEBUG: now have {} verts", verts.len()); - println!("DEBUG: verts={:?}", verts); - - println!("DEBUG: offset={} mapping={:?}", offset, mapping); - println!("DEBUG: remap={:?}", remap); - - // All faces need copied, but if the index was to - // a concrete vertex, then it needs shifted by 'offset'; - // if an argument, it needs remapped. - faces.extend(child.faces.iter().enumerate().map(|(i,n)| { + // Also, we didn't copy Arg elements from 'child.verts', but + // we do use them below - if 'child.faces' makes a + // reference to this Arg, we resolve it with the help of + // 'arg_vals', passed in by the caller. + faces.extend(child.faces.iter().map(|n| { let f = match child.verts[*n] { - VertexUnion::Vertex(_) => remap[*n] + offset, - VertexUnion::Arg(m) => mapping[m], + VertexUnion::Vertex(_) => remap[*n], + VertexUnion::Arg(m) => arg_vals[m], }; - println!("face at i={} (n={}): new f={}, {} verts; vert={:?} -> {:?}", i, n, f, verts.len(), child.verts[*n], verts[f]); if f >= verts.len() { panic!("face >= num_verts") } f })); - let o2 = remap.iter().map(|n| n + offset).collect(); - offsets.push(o2); + // Since the caller might need this same remapping (there + // could be an Arg that referred to these vertices), we + // save this information to return it later: + remaps.push(remap); } let m = MeshFunc { verts: verts, faces: faces, }; - (m, offsets) + (m, remaps) } } diff --git a/src/rule.rs b/src/rule.rs index 7626659..cea4a54 100644 --- a/src/rule.rs +++ b/src/rule.rs @@ -283,9 +283,7 @@ impl Rule { // (We pass a one-element vector to geom.connect() above // so remaps always has just one element.) for child in eval.children.iter_mut() { - println!("DEBUG: got shifting child.arg_vals={:?}", child.arg_vals); child.arg_vals = child.arg_vals.iter().map(|n| remap[*n]).collect(); - println!("DEBUG: new child.arg_vals={:?}", child.arg_vals); } // We're done evaluating this rule, so increment 'next'.