Fix hybrid file parsing (#33)

Fix parsing hybrid files containing both drawing commands and sub-file
references. Stop trying to guess based on sub-file extension.

Fixes #32

Author: djeedai

.github/workflows/ci.yml

@@ -19,7 +19,7 @@ jobs:
         rust:
           - stable
           - beta
-          - 1.58.1
+          - 1.61.0
 
     steps:
     - name: Checkout

README.md

@@ -3,7 +3,7 @@
 [![License: MIT or Apache 2.0](https://img.shields.io/badge/License-MIT%20or%20Apache2-blue.svg)](https://github.com/djeedai/weldr#license)
 [![CI](https://github.com/djeedai/weldr/workflows/CI/badge.svg?branch=main)](https://github.com/djeedai/weldr/actions?query=workflow%3ACI)
 [![Coverage Status](https://coveralls.io/repos/github/djeedai/weldr/badge.svg?branch=main)](https://coveralls.io/github/djeedai/weldr?branch=main)
-[![Minimum rustc version](https://img.shields.io/badge/rustc-1.56.0+-lightgray.svg)](#rust-version-requirements)
+![Minimum rustc version](https://img.shields.io/badge/rustc-1.61.0+-lightgray.svg)
 
 weldr is a Rust library and command-line tool to manipulate [LDraw](https://www.ldraw.org/) files ([format specification](https://www.ldraw.org/article/218.html)), which are files describing 3D models of [LEGO®](http://www.lego.com)* pieces.
 
@@ -21,8 +21,6 @@ The weldr library allows building command-line tools and applications leveraging
 Parse the content of a single LDraw file containing 2 commands:
 
 ```rust
-extern crate weldr;
-
 use weldr::{parse_raw, CommandType, CommentCmd, LineCmd, Vec3};
 
 fn main() {}

bin/weldr/src/convert.rs

@@ -124,41 +124,38 @@ impl ConvertCommand {
         };
         gltf.nodes.push(node);
 
-        // TODO: Check the part type rather than the extension.
-        if filename.ends_with(".dat") {
-            // Create geometry if the node is a part.
-            let mesh_index = mesh_cache.entry(filename.into()).or_insert_with(|| {
-                let mesh_index = gltf.meshes.len() as u32;
-                let geometry = self.create_geometry(source_file, source_map);
-                // Don't set empty meshes to avoid import errors.
-                if !geometry.vertices.is_empty() && !geometry.triangle_indices.is_empty() {
-                    self.add_mesh(&geometry, gltf, buffer);
-                    Some(mesh_index)
-                } else {
-                    None
-                }
-            });
-
-            gltf.nodes[node_index].mesh_index = *mesh_index;
-        } else {
-            for cmd in &source_file.cmds {
-                if let Command::SubFileRef(sfr_cmd) = cmd {
-                    if let Some(subfile) = source_map.get(&sfr_cmd.file) {
-                        // Don't apply node transforms to preserve the scene hierarchy.
-                        // Applications should handle combining the transforms.
-                        let transform = sfr_cmd.matrix();
-
-                        let child_node_index = self.add_nodes(
-                            &sfr_cmd.file,
-                            subfile,
-                            Some(transform),
-                            source_map,
-                            gltf,
-                            buffer,
-                            mesh_cache,
-                        );
-                        gltf.nodes[node_index].children.push(child_node_index);
-                    }
+        // Create geometry if any for this node
+        let opt_mesh_index = mesh_cache.entry(filename.into()).or_insert_with(|| {
+            let mesh_index = gltf.meshes.len() as u32;
+            let geometry = self.create_geometry(source_file, source_map);
+            // Don't set empty meshes to avoid import errors.
+            if !geometry.vertices.is_empty() && !geometry.triangle_indices.is_empty() {
+                self.add_mesh(&geometry, gltf, buffer);
+                Some(mesh_index)
+            } else {
+                None
+            }
+        });
+        gltf.nodes[node_index].mesh_index = *opt_mesh_index;
+
+        // Recursively parse sub-files
+        for cmd in &source_file.cmds {
+            if let Command::SubFileRef(sfr_cmd) = cmd {
+                if let Some(subfile) = source_map.get(&sfr_cmd.file) {
+                    // Don't apply node transforms to preserve the scene hierarchy.
+                    // Applications should handle combining the transforms.
+                    let transform = sfr_cmd.matrix();
+
+                    let child_node_index = self.add_nodes(
+                        &sfr_cmd.file,
+                        subfile,
+                        Some(transform),
+                        source_map,
+                        gltf,
+                        buffer,
+                        mesh_cache,
+                    );
+                    gltf.nodes[node_index].children.push(child_node_index);
                 }
             }
         }

bin/weldr/src/weldr.rs

@@ -305,13 +305,15 @@ impl GeometryCache {
     }
 
     fn add_line(&mut self, draw_ctx: &DrawContext, vertices: &[Vec3; 2]) {
+        trace!("LINE: {:?}", vertices);
         let i0 = self.insert_vertex(&vertices[0], &draw_ctx.transform);
         let i1 = self.insert_vertex(&vertices[1], &draw_ctx.transform);
         self.line_indices.push(i0);
         self.line_indices.push(i1);
     }
 
     fn add_triangle(&mut self, draw_ctx: &DrawContext, vertices: &[Vec3; 3]) {
+        trace!("TRI: {:?}", vertices);
         let i0 = self.insert_vertex(&vertices[0], &draw_ctx.transform);
         let i1 = self.insert_vertex(&vertices[1], &draw_ctx.transform);
         let i2 = self.insert_vertex(&vertices[2], &draw_ctx.transform);
@@ -321,8 +323,17 @@ impl GeometryCache {
     }
 
     fn add_quad(&mut self, draw_ctx: &DrawContext, vertices: &[Vec3; 4]) {
-        self.add_triangle(draw_ctx, &[vertices[0], vertices[1], vertices[2]]);
-        self.add_triangle(draw_ctx, &[vertices[0], vertices[2], vertices[3]]);
+        trace!("QUAD: {:?}", vertices);
+        let i0 = self.insert_vertex(&vertices[0], &draw_ctx.transform);
+        let i1 = self.insert_vertex(&vertices[1], &draw_ctx.transform);
+        let i2 = self.insert_vertex(&vertices[2], &draw_ctx.transform);
+        let i3 = self.insert_vertex(&vertices[3], &draw_ctx.transform);
+        self.triangle_indices.push(i0);
+        self.triangle_indices.push(i2);
+        self.triangle_indices.push(i1);
+        self.triangle_indices.push(i0);
+        self.triangle_indices.push(i3);
+        self.triangle_indices.push(i2);
     }
 }
 

lib/tests/parse.rs

@@ -1,5 +1,3 @@
-extern crate weldr;
-
 use weldr::{error::ResolveError, Command, FileRefResolver, SourceFile, SourceMap, SubFileRefCmd};
 
 use std::collections::HashMap;
@@ -67,7 +65,7 @@ fn test_memory_resolver() {
 #[test]
 fn parse_recursive() {
     let mut memory_resolver = MemoryResolver::new();
-    memory_resolver.add("root.ldr", b"1 16 0 0 0 1 0 0 0 1 0 0 0 1 a.ldr\n1 16 0 0 0 1 0 0 0 1 0 0 0 1 b.ldr\n1 16 0 0 0 1 0 0 0 1 0 0 0 1 a.ldr");
+    memory_resolver.add("root.ldr", b"1 16 0 0 0 1 0 0 0 1 0 0 0 1 a.ldr\n4 16 0 0 0 1 1 1 2 2 2 3 3 3\n1 16 0 0 0 1 0 0 0 1 0 0 0 1 b.ldr\n1 16 0 0 0 1 0 0 0 1 0 0 0 1 a.ldr");
     memory_resolver.add("a.ldr", b"4 16 1 1 0 0.9239 1 0.3827 0.9239 0 0.3827 1 0 0");
     memory_resolver.add(
         "b.ldr",
@@ -76,16 +74,18 @@ fn parse_recursive() {
     let mut source_map = weldr::SourceMap::new();
     let root_file_name = weldr::parse("root.ldr", &memory_resolver, &mut source_map).unwrap();
     let root_file = source_map.get(&root_file_name).unwrap();
-    assert_eq!(3, root_file.cmds.len());
+    assert_eq!(4, root_file.cmds.len());
+    // Regression #32: top-level drawing commands work with sub-file references
+    assert!(matches!(root_file.cmds[1], Command::Quad(_)));
 
     let file0 = get_resolved_subfile_ref(&root_file.cmds[0]).unwrap();
     assert_eq!(file0.file, "a.ldr");
     assert_eq!(1, source_map.get(&file0.file).unwrap().cmds.len());
 
-    let file1 = get_resolved_subfile_ref(&root_file.cmds[1]).unwrap();
+    let file1 = get_resolved_subfile_ref(&root_file.cmds[2]).unwrap();
     assert_eq!(2, source_map.get(&file1.file).unwrap().cmds.len());
 
-    let file2 = get_resolved_subfile_ref(&root_file.cmds[2]).unwrap();
+    let file2 = get_resolved_subfile_ref(&root_file.cmds[3]).unwrap();
     assert_eq!(1, source_map.get(&file2.file).unwrap().cmds.len());
     assert_eq!(file0, file2);