Fixed bug introduced by the new PolySet cache; string filtering done in the wrong place, refactored cache into separate class

openscad.pro

@@ -159,6 +159,7 @@ HEADERS += src/renderer.h \
            src/traverser.h \
            src/nodecache.h \
            src/nodedumper.h \
+           src/PolySetCache.h \
            src/PolySetEvaluator.h \
            src/CSGTermEvaluator.h \
            src/myqhash.h \
@@ -208,6 +209,7 @@ SOURCES += src/openscad.cc \
            src/qhash.cc \
            src/Tree.cc \
 	   src/mathc99.cc \
+           src/PolySetCache.cc \
            src/PolySetEvaluator.cc
 
 cgal {

src/PolySetCache.cc

@@ -0,0 +1,21 @@
+#include "PolySetCache.h"
+#include "printutils.h"
+#include "PolySet.h"
+
+PolySetCache *PolySetCache::inst = NULL;
+
+void PolySetCache::insert(const std::string &id, const shared_ptr<PolySet> &ps)
+{
+	this->cache.insert(id, new cache_entry(ps), ps ? ps->polygons.size() : 0);
+}
+
+void PolySetCache::print()
+{
+	PRINTF("PolySets in cache: %d", this->cache.size());
+	PRINTF("Polygons in cache: %d", this->cache.totalCost());
+}
+
+PolySetCache::cache_entry::cache_entry(const shared_ptr<PolySet> &ps) : ps(ps)
+{
+	if (print_messages_stack.size() > 0) this->msg = print_messages_stack.last();
+}

src/PolySetCache.h

@@ -0,0 +1,34 @@
+#ifndef POLYSETCACHE_H_
+#define POLYSETCACHE_H_
+
+#include "myqhash.h"
+#include <QCache>
+#include "memory.h"
+
+class PolySetCache
+{
+public:	
+	PolySetCache(size_t polygonlimit = 100000) : cache(polygonlimit) {}
+
+	static PolySetCache *instance() { if (!inst) inst = new PolySetCache; return inst; }
+
+	bool contains(const std::string &id) const { return this->cache.contains(id); }
+	shared_ptr<class PolySet> get(const std::string &id) const { return this->cache[id]->ps; }
+	void insert(const std::string &id, const shared_ptr<PolySet> &ps);
+	void clear() { cache.clear(); }
+	void print();
+
+private:
+	static PolySetCache *inst;
+
+	struct cache_entry {
+		shared_ptr<class PolySet> ps;
+		QString msg;
+		cache_entry(const shared_ptr<PolySet> &ps);
+		~cache_entry() { }
+	};
+
+	QCache<std::string, cache_entry> cache;
+};
+
+#endif

src/PolySetEvaluator.cc

@@ -1,3 +1,4 @@
+#include "PolySetCache.h"
 #include "PolySetEvaluator.h"
 #include "printutils.h"
 #include "polyset.h"
@@ -9,7 +10,10 @@
 	class.
 */
 
-QCache<std::string, PolySetEvaluator::cache_entry> PolySetEvaluator::cache(100000);
+static bool filter(char c)
+{
+	return c == ' ' || c == '\n' || c == '\t' || c == '\r';
+}
 
 /*!
   Factory method returning a PolySet from the given node. If the
@@ -20,23 +24,15 @@ QCache<std::string, PolySetEvaluator::cache_entry> PolySetEvaluator::cache(10000
 shared_ptr<PolySet> PolySetEvaluator::getPolySet(const AbstractNode &node, bool cache)
 {
 	std::string cacheid = this->tree.getString(node);
-	if (this->cache.contains(cacheid)) {
-		PRINTF("Cache hit: %s", cacheid.substr(0, 40).c_str());
-		return this->cache[cacheid]->ps;
+	cacheid.erase(std::remove_if(cacheid.begin(), cacheid.end(), filter), cacheid.end());
+
+	if (PolySetCache::instance()->contains(cacheid)) {
+// For cache debugging
+//		PRINTF("Cache hit: %s", cacheid.substr(0, 40).c_str());
+		return PolySetCache::instance()->get(cacheid);
 	}
 
 	shared_ptr<PolySet> ps(node.evaluate_polyset(this));
-	if (cache) this->cache.insert(cacheid, new cache_entry(ps), ps?ps->polygons.size():0);
+	if (cache) PolySetCache::instance()->insert(cacheid, ps);
 	return ps;
 }
-
-PolySetEvaluator::cache_entry::cache_entry(const shared_ptr<PolySet> &ps) : ps(ps)
-{
-	if (print_messages_stack.size() > 0) this->msg = print_messages_stack.last();
-}
-
-void PolySetEvaluator::printCache()
-{
-	PRINTF("PolySets in cache: %d", cache.size());
-	PRINTF("Polygons in cache: %d", cache.totalCost());
-}

src/PolySetEvaluator.h

@@ -1,10 +1,8 @@
 #ifndef POLYSETEVALUATOR_H_
 #define POLYSETEVALUATOR_H_
 
-#include "myqhash.h"
 #include "node.h"
 #include "Tree.h"
-#include <QCache>
 #include "memory.h"
 
 class PolySetEvaluator
@@ -23,21 +21,6 @@ public:
 	virtual PolySet *evaluatePolySet(const class CgaladvNode &) { return NULL; }
 	virtual PolySet *evaluatePolySet(const class RenderNode &) { return NULL; }
 
-	static void clearCache() {
-		cache.clear();
-	}
-	void printCache();
-protected:
-
-	struct cache_entry {
-		shared_ptr<class PolySet> ps;
-		QString msg;
-		cache_entry(const shared_ptr<PolySet> &ps);
-		~cache_entry() { }
-	};
-
-	static QCache<std::string, cache_entry> cache;
-
 private:
 	const Tree &tree;
 };

src/Tree.cc

@@ -4,16 +4,11 @@
 #include <assert.h>
 #include <algorithm>
 
-static bool filter(char c)
-{
-	return c == ' ' || c == '\n' || c == '\t' || c == '\r';
-}
-
 /*!
 	Returns the cached string representation of the subtree rooted by \a node.
 	If node is not cached, the cache will be rebuilt.
 */
-const std::string Tree::getString(const AbstractNode &node) const
+const std::string &Tree::getString(const AbstractNode &node) const
 {
 	assert(this->root_node);
 	if (!this->nodecache.contains(node)) {
@@ -23,10 +18,7 @@ const std::string Tree::getString(const AbstractNode &node) const
 		assert(this->nodecache.contains(*this->root_node) &&
 					 "NodeDumper failed to create a cache");
 	}
-	std::string str = this->nodecache[node];
-	str.erase(std::remove_if(str.begin(), str.end(), filter), str.end());
-
-	return str;
+	return this->nodecache[node];
 }
 
 /*!

src/Tree.h

@@ -20,7 +20,7 @@ public:
 	void setRoot(const AbstractNode *root);
 	const AbstractNode *root() const { return this->root_node; }
 
-	const string getString(const AbstractNode &node) const;
+	const string &getString(const AbstractNode &node) const;
 
 private:
 	const AbstractNode *root_node;

src/mainwin.cc

@@ -24,6 +24,7 @@
  *
  */
 
+#include "PolySetCache.h"
 #include "MainWindow.h"
 #include "openscad.h" // examplesdir
 #include "Preferences.h"
@@ -790,7 +791,7 @@ void MainWindow::compileCSG(bool procevents)
 			if (procevents)
 				QApplication::processEvents();
 		}
-		psevaluator.printCache();
+		PolySetCache::instance()->print();
 	}
 	catch (ProgressCancelException e) {
 		PRINT("CSG generation cancelled.");
@@ -1226,7 +1227,7 @@ void MainWindow::actionRenderCGAL()
 		QHash<std::string, CGAL_Nef_polyhedron> cache;
 		CGALEvaluator evaluator(cache, this->tree);
 		this->root_N = new CGAL_Nef_polyhedron(evaluator.evaluateCGALMesh(*this->root_node));
-		evaluator.psevaluator.printCache();
+		PolySetCache::instance()->print();
 	}
 	catch (ProgressCancelException e) {
 		PRINT("Rendering cancelled.");
@@ -1497,7 +1498,7 @@ void MainWindow::actionExportImage()
 void MainWindow::actionFlushCaches()
 {
 // FIXME: Polycache -> PolySetEvaluator
-	PolySetEvaluator::clearCache();
+	PolySetCache::instance()->clear();
 #ifdef ENABLE_CGAL
 // FIXME: Flush caches through whatever channels we have
 	// CGALEvaluator::evaluator()->getCache().clear();

tests/CMakeLists.txt

@@ -110,6 +110,7 @@ set(COMMON_SOURCES
   ../src/nodedumper.cc 
   ../src/traverser.cc 
   ../src/PolySetEvaluator.cc 
+  ../src/PolySetCache.cc 
   ../src/Tree.cc
   ${FLEX_OpenSCADlexer_OUTPUTS}
   ${BISON_OpenSCADparser_OUTPUTS})

tests/dumptest.cc

@@ -144,7 +144,7 @@ int main(int argc, char **argv)
 
 	string dumpstdstr = tree.getString(*root_node);
 	string dumpstdstr_cached = tree.getString(*root_node);
-	if (dumpstdstr != dumpstdstr_cached) rc = 1;
+	assert(dumpstdstr == dumpstdstr_cached);
 
 	std::cout << dumpstdstr << "\n";