Support empty id attribtes and layer names.

- This is not possible to setup via Inkscape GUI
- Add warning if a given filter results in no match

src/core/ImportNode.cc

@@ -108,15 +108,11 @@ static std::shared_ptr<AbstractNode> do_import(const ModuleInstantiation *inst,
     if (layername.isDefined()) {
       LOG(message_group::Deprecated, Location::NONE, "", "layername= is deprecated. Please use layer=");
       node->layer = layername.toString();
-    } else {
-      node->layer = "";
     }
   }
   const auto& idval = parameters["id"];
   if (idval.isDefined()) {
     node->id = idval.toString();
-  } else {
-    node->id = "";
   }
   node->convexity = (int)parameters["convexity"].toDouble();
 
@@ -201,7 +197,7 @@ const Geometry *ImportNode::createGeometry() const
     break;
   }
   case ImportType::DXF: {
-    DxfData dd(this->fn, this->fs, this->fa, this->filename, this->layer, this->origin_x, this->origin_y, this->scale);
+    DxfData dd(this->fn, this->fs, this->fa, this->filename, this->layer.value_or(""), this->origin_x, this->origin_y, this->scale);
     g = dd.toPolygon2d();
     break;
   }
@@ -226,10 +222,12 @@ std::string ImportNode::toString() const
   fs::path path((std::string)this->filename);
 
   stream << this->name();
-  stream << "(file = " << this->filename
-         << ", layer = " << QuotedString(this->layer);
-  if (!this->id.empty()) {
-    stream << ", id = " << QuotedString(this->id);
+  stream << "(file = " << this->filename;
+  if (this->id) {
+    stream << ", id = " << QuotedString(this->id.get());
+  }
+  if (this->layer) {
+    stream << ", layer = " << QuotedString(this->layer.get());
   }
   stream << ", origin = [" << std::dec << this->origin_x << ", " << this->origin_y << "]";
   if (this->type == ImportType::SVG) {

src/core/ImportNode.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <boost/optional.hpp>
+
 #include "node.h"
 #include "Value.h"
 
@@ -26,8 +28,8 @@ class ImportNode : public LeafNode
 
   ImportType type;
   Filename filename;
-  std::string layer;
-  std::string id;
+  boost::optional<std::string> id;
+  boost::optional<std::string> layer;
   int convexity;
   bool center;
   double dpi;

src/io/import.h

@@ -1,15 +1,18 @@
 #pragma once
 
 #include <string>
+#include <boost/optional.hpp>
+
 #include "AST.h"
 
 class PolySet *import_stl(const std::string& filename, const Location& loc);
 
 PolySet *import_off(const std::string& filename, const Location& loc);
 
 class Polygon2d *import_svg(double fn, double fs, double fa,
-        const std::string& filename, const std::string& id, const std::string& layer,
-        const double dpi, const bool center, const Location& loc);
+                            const std::string& filename,
+                            const boost::optional<std::string>& id, const boost::optional<std::string>& layer,
+                            const double dpi, const bool center, const Location& loc);
 
 #ifdef ENABLE_CGAL
 class CGAL_Nef_polyhedron *import_nef3(const std::string& filename, const Location& loc);

src/io/import_svg.cc

@@ -84,37 +84,50 @@ double calc_alignment(const libsvg::align_t alignment, double page_mm, double sc
 
 
 Polygon2d *import_svg(double fn, double fs, double fa,
-                      const std::string& filename, const std::string& id, const std::string& layer,
+                      const std::string& filename,
+                      const boost::optional<std::string>& id, const boost::optional<std::string>& layer,
                       const double dpi, const bool center, const Location& loc)
 {
   try {
     fnContext scadContext(fn, fs, fa);
-    if (!id.empty()) {
-      scadContext.selector = [id, layer](const libsvg::shape *s) {
+    if (id) {
+      scadContext.selector = [&scadContext, id, layer](const libsvg::shape *s) {
           bool layer_match = true;
-          if (!layer.empty()) {
-              layer_match = false;
-              for (const libsvg::shape *shape = s;shape->get_parent() != nullptr;shape = shape->get_parent()) {
-                  if (shape->get_layer() == layer) {
-                      layer_match = true;
-                      break;
-                  }
+          if (layer) {
+            layer_match = false;
+            for (const libsvg::shape *shape = s; shape->get_parent() != nullptr; shape = shape->get_parent()) {
+              if (shape->has_layer() && shape->get_layer() == layer.get()) {
+                layer_match = true;
+                break;
               }
+            }
           }
-          return layer_match && s->get_id() == id;
+          return scadContext.match(layer_match && s->has_id() && s->get_id() == id.get());
         };
-    } else if (!layer.empty()) {
-      scadContext.selector = [layer](const libsvg::shape *s) {
-          return s->get_layer() == layer;
+    } else if (layer) {
+      scadContext.selector = [&scadContext, layer](const libsvg::shape *s) {
+          return scadContext.match(s->has_layer() && s->get_layer() == layer.get());
         };
     } else {
       // no selection means selecting the root
-      scadContext.selector = [](const libsvg::shape *s) {
-          return s->get_parent() == nullptr;
+      scadContext.selector = [&scadContext](const libsvg::shape *s) {
+          return scadContext.match(s->get_parent() == nullptr);
         };
     }
 
+    std::string match_args;
+    if (id) {
+      match_args += "id = \"" + id.get() + "\"";
+    }
+    if (layer) {
+      if (id) match_args += ", ";
+      match_args += "layer = \"" + layer.get() + "\"";
+    }
+
     const auto shapes = libsvg::libsvg_read_file(filename.c_str(), (void *) &scadContext);
+    if (!match_args.empty() && !scadContext.has_matches()) {
+      LOG(message_group::Warning, loc, "", "import() filter %2$s did not match anything", filename, match_args);
+    }
 
     double width_mm = 0.0;
     double height_mm = 0.0;
@@ -139,7 +152,7 @@ Polygon2d *import_svg(double fn, double fs, double fa,
         if (viewbox_valid) {
           double px = w.unit == libsvg::unit_t::PERCENT ? w.number / 100.0 : 1.0;
           double py = h.unit == libsvg::unit_t::PERCENT ? h.number / 100.0 : 1.0;
-          viewbox << px * page->get_viewbox().x, py * page->get_viewbox().y;
+          viewbox << px * page->get_viewbox().x, py *page->get_viewbox().y;
 
           scale << width_mm / page->get_viewbox().width,
             height_mm / page->get_viewbox().height;

src/io/libsvg/libsvg.cc

@@ -128,7 +128,7 @@ void processNode(xmlTextReaderPtr reader, shapes_defs_list_t *defs_lookup_list,
         if (!in_defs) {
           shape_list->push_back(s);
         } else {
-          if (!s->get_id().empty()) {
+          if (!s->get_id_or_default().empty()) {
             defs_lookup_list->insert(std::make_pair(s->get_id(), s));
           }
           temp_defs_storage->push_back(s);

src/io/libsvg/shape.cc

@@ -97,7 +97,9 @@ shape::create_from_name(const char *name)
 void
 shape::set_attrs(attr_map_t& attrs, void *context)
 {
-  this->id = attrs["id"];
+  if (attrs.find("id") != attrs.end()) {
+    this->id = attrs["id"];
+  }
   this->transform = attrs["transform"];
   this->stroke_width = attrs["stroke-width"];
   this->stroke_linecap = attrs["stroke-linecap"];
@@ -115,10 +117,9 @@ shape::set_attrs(attr_map_t& attrs, void *context)
     excluded = true;
   }
 
-  const std::string inkscape_label = attrs["inkscape:label"];
   const std::string inkscape_groupmode = attrs["inkscape:groupmode"];
-  if (inkscape_groupmode == "layer" && !inkscape_label.empty()) {
-      this->layer = inkscape_label;
+  if (inkscape_groupmode == "layer" && attrs.find("inkscape:label") != attrs.end()) {
+    this->layer = attrs["inkscape:label"];
   }
 
   const fnContext *ctx = reinterpret_cast<const fnContext *>(context);
@@ -349,7 +350,7 @@ shape::clone_children() {
 
 std::ostream& operator<<(std::ostream& os, const shape& s)
 {
-  return os << s.dump() << " | id = '" << s.id << "', transform = '" << s.transform << "'";
+  return os << s.dump() << " | id = '" << s.id.value_or("") << "', transform = '" << s.transform << "'";
 }
 
 } // namespace libsvg

src/io/libsvg/shape.h

@@ -25,6 +25,7 @@
 #pragma once
 
 #include <map>
+#include <atomic>
 #include <string>
 #include <vector>
 #include <memory>
@@ -35,6 +36,8 @@
 #include <Eigen/Core>
 #include <Eigen/Geometry>
 
+#include <boost/optional.hpp>
+
 #include "util.h"
 #include "ext/polyclipping/clipper.hpp"
 
@@ -44,17 +47,18 @@ class shape;
 
 // ccox - I don't like putting this here, but the svg library code did not plan ahead for app customization.
 // And this is one of the few sensible places to put it without adding new header files.
-typedef struct fnContext {
-public:
+struct fnContext {
   fnContext(double fNN, double fSS, double fAA) : fn(fNN), fs(fSS), fa(fAA) {}
+  bool match(bool val) { if (val) matches++; return val; }
+  bool has_matches() { return matches.load() > 0; }
 
-public:
   double fn;
   double fs;
   double fa;
   std::function<bool (const libsvg::shape *)> selector;
-} fnContext;
-
+private:
+  std::atomic<int> matches{0};
+};
 
 namespace libsvg {
 
@@ -69,7 +73,8 @@ class shape
   std::vector<shape *> children;
 
 protected:
-  std::string id;
+  boost::optional<std::string> id;
+  boost::optional<std::string> layer;
   double x;
   double y;
   path_list_t path_list;
@@ -78,7 +83,6 @@ class shape
   std::string stroke_linecap;
   std::string stroke_linejoin;
   std::string style;
-  std::string layer;
   bool excluded;
   bool selected;
 
@@ -99,8 +103,11 @@ class shape
   virtual void add_child(shape *s) { children.push_back(s); s->set_parent(this); }
   virtual const std::vector<shape *>& get_children() const { return children; }
 
-  virtual const std::string& get_id() const { return id; }
-  virtual const std::string& get_layer() const { return layer; }
+  virtual bool has_id() const { return id.is_initialized(); }
+  virtual const std::string& get_id() const { return id.get(); }
+  virtual const std::string get_id_or_default(const std::string& def = "") const { return id.get_value_or(def); }
+  virtual bool has_layer() const { return layer.is_initialized(); }
+  virtual const std::string& get_layer() const { return layer.get(); }
   virtual double get_x() const { return x; }
   virtual double get_y() const { return y; }
 

src/io/libsvg/use.cc

@@ -59,7 +59,7 @@ use::set_attrs(attr_map_t& attrs, void *context)
   if (this->href != temp_href) {
     this->href = temp_href;
     if (href.rfind("#", 0) != 0) {
-      printf("<use> can only use references to ids in the href field (starting with #). Error in element type %s with id: %s\n", this->get_name().c_str(), this->get_id().c_str());
+      printf("<use> can only use references to ids in the href field (starting with #). Error in element type %s with id: %s\n", this->get_name().c_str(), this->get_id_or_default().c_str());
     }
   }
 

tests/CMakeLists.txt

@@ -492,7 +492,7 @@ file(GLOB SCAD_PDF_FILES      ${TEST_SCAD_DIR}/pdf/*.scad)
 file(GLOB SCAD_SVG_FILES      ${TEST_SCAD_DIR}/svg/svg-spec/*.scad
   ${TEST_SCAD_DIR}/svg/box-w-holes-2d.scad
   ${TEST_SCAD_DIR}/svg/display.scad
-  ${TEST_SCAD_DIR}/svg/id.scad
+  ${TEST_SCAD_DIR}/svg/id-selection-test.scad
   ${TEST_SCAD_DIR}/svg/id-layer-selection-test.scad
   ${TEST_SCAD_DIR}/svg/line-cap-line-join.scad
   ${TEST_SCAD_DIR}/svg/simple-center-2d.scad
@@ -646,6 +646,7 @@ list(APPEND ASTDUMPTEST_FILES ${MISC_FILES}
   ${TEST_SCAD_DIR}/functions/list-comprehensions.scad
   ${TEST_SCAD_DIR}/functions/exponent-operator-test.scad
   ${TEST_SCAD_DIR}/misc/ifelse-ast-dump.scad
+  ${TEST_SCAD_DIR}/svg/id-layer-selection-test.scad
 )
 
 list(APPEND DUMPTEST_FILES ${FEATURES_2D_FILES} ${FEATURES_3D_FILES} ${DEPRECATED_3D_FILES} ${MISC_FILES})

tests/data/scad/svg/id-layer-selection-test.scad

@@ -1,5 +1,6 @@
 svg="../../svg/id-layer-selection-test.svg";
 module test(nr, pos) translate(pos) {
+	echo("test ", str(nr));
 	difference() {
 		children(0);
 		translate([2, 2]) text(str(nr));
@@ -9,12 +10,18 @@ module test(nr, pos) translate(pos) {
 }
 module show(c) color(c) offset(0.01) children();
 
+echo(svg=svg);
+import(svg, id="unknown id");
+import(svg, layer="unknown layer");
+import(svg, id="unknown id", layer="unknown layer");
+
 test(1, [0, 0]) {
 	show("red")    import(svg, layer="layer-1");
 	show("green")  import(svg, layer="layer-2");
 	show("yellow") import(svg, layer="stars");
 	// not selected, group-1 is not a layer
 	show("blue")   import(svg, layer="group-1");
+	show("white")  import(svg, layer="");
 }
 
 test(2, [0, 60]) {
@@ -23,6 +30,7 @@ test(2, [0, 60]) {
 	show("yellow") import(svg, id="id-star-1");
 	show("yellow") import(svg, id="id-star-2");
 	show("blue")   import(svg, id="id-boxes");
+	show("white")  import(svg, id="id-ellipse");
 }
 
 test(3, [120, 0]) {
@@ -38,8 +46,9 @@ test(4, [120, 60]) {
 	// not selected, layer not matching
 	show("green")  import(svg, id="id-circle", layer="N/A");
 	show("yellow") import(svg, id="id-star-3", layer="stars");
-	// selected, empty string as layer is treated as not specified
+	// not selected, empty string as layer is not same as not specified
 	show("blue")   import(svg, id="id-group-1", layer="");
 	// not selected, layer not matching
 	show("cyan")   import(svg, id="id-group-2", layer="N/A");
+	show("white")  import(svg, id="id-ellipse", layer="");
 }

tests/data/scad/svg/id.scad → tests/data/scad/svg/id-selection-test.scad

@@ -2,13 +2,14 @@
 translate([  0, 400]) import("../../svg/display.svg", id="rect11");
 // selection overrides invisibility of single element
 translate([200, 400]) import("../../svg/display.svg", id="rect13");
-// empty is the default and selects everything
-translate([400, 400]) import("../../svg/display.svg", id="");
+// undef is the default and selects everything
+translate([400, 400]) import("../../svg/display.svg", id=undef);
 // group (internal invisibilities respected)
 translate([  0, 200]) import("../../svg/display.svg", id="group1");
 // selection overrides invisibility of group
 translate([200, 200]) import("../../svg/display.svg", id="group3");
 // unknown id selects nothing
+translate([400, 200]) import("../../svg/display.svg", id="");
 translate([400, 200]) import("../../svg/display.svg", id="doesnotexist");
 // id from defs selects all instances
 translate([  0,   0]) import("../../svg/display.svg", id="tworects");