Fixed normalization bug: node limit wasn't checked properly and some cases triggered exponential expansion of nodes during normalization. Fixes #127

src/csgtermnormalizer.cc

@@ -5,25 +5,29 @@
 /*!
 	NB! for e.g. empty intersections, this can normalize a tree to nothing and return NULL.
 */
-shared_ptr<CSGTerm> CSGTermNormalizer::normalize(const shared_ptr<CSGTerm> &root, 
+shared_ptr<CSGTerm> CSGTermNormalizer::normalize(const shared_ptr<CSGTerm> &root)
-																								 size_t limit)
 {
 	shared_ptr<CSGTerm> temp = root;
 	while (1) {
+		this->rootnode = temp;
+		this->nodecount = 0;
 		shared_ptr<CSGTerm> n = normalizePass(temp);
 		if (!n) return n; // If normalized to nothing
 		if (temp == n) break;
 		temp = n;
 
-		unsigned int num = count(temp);
+		if (this->nodecount > this->limit) {
-#ifdef DEBUG
+			PRINTB("WARNING: Normalized tree is growing past %d elements. Aborting normalization.\n", this->limit);
-		PRINTB("Normalize count: %d\n", num);
+      // Clean up any partially evaluated terms
-#endif
+			shared_ptr<CSGTerm> newroot = root, tmproot;
-		if (num > limit) {
+			while (newroot != tmproot) {
-			PRINTB("WARNING: Normalized tree is growing past %d elements. Aborting normalization.\n", limit);
+				tmproot = newroot;
-			return root;
+				newroot = collapse_null_terms(tmproot);
+			}
+			return newroot;
 		}
 	}
+	this->rootnode.reset();
 	return temp;
 }
 
@@ -42,7 +46,11 @@ shared_ptr<CSGTerm> CSGTermNormalizer::normalizePass(shared_ptr<CSGTerm> term)
 	}
 
 	do {
-		while (term && normalize_tail(term)) { }
+		while (term && match_and_replace(term)) {	}
+		this->nodecount++;
+		if (nodecount > this->limit) {
+			return shared_ptr<CSGTerm>();
+		}
 		if (!term || term->type == CSGTerm::TYPE_PRIMITIVE) return term;
 		if (term->left) term->left = normalizePass(term->left);
 	} while (term->type != CSGTerm::TYPE_UNION &&
@@ -51,6 +59,11 @@ shared_ptr<CSGTerm> CSGTermNormalizer::normalizePass(shared_ptr<CSGTerm> term)
 	term->right = normalizePass(term->right);
 
 	// FIXME: Do we need to take into account any transformation of item here?
+	return collapse_null_terms(term);
+}
+
+shared_ptr<CSGTerm> CSGTermNormalizer::collapse_null_terms(const shared_ptr<CSGTerm> &term)
+{
 	if (!term->right) {
 		if (term->type == CSGTerm::TYPE_UNION || term->type == CSGTerm::TYPE_DIFFERENCE) return term->left;
 		else return term->right;
@@ -59,13 +72,14 @@ shared_ptr<CSGTerm> CSGTermNormalizer::normalizePass(shared_ptr<CSGTerm> term)
 		if (term->type == CSGTerm::TYPE_UNION) return term->right;
 		else return term->left;
 	}
-
 	return term;
 }
 
-bool CSGTermNormalizer::normalize_tail(shared_ptr<CSGTerm> &term)
+bool CSGTermNormalizer::match_and_replace(shared_ptr<CSGTerm> &term)
 {
-	if (term->type == CSGTerm::TYPE_UNION || term->type == CSGTerm::TYPE_PRIMITIVE) return false;
+	if (term->type == CSGTerm::TYPE_UNION || term->type == CSGTerm::TYPE_PRIMITIVE) {
+		return false;
+	}
 
 	// Part A: The 'x . (y . z)' expressions
 
@@ -149,8 +163,9 @@ bool CSGTermNormalizer::normalize_tail(shared_ptr<CSGTerm> &term)
 	return false;
 }
 
+// Counts all non-leaf nodes
 unsigned int CSGTermNormalizer::count(const shared_ptr<CSGTerm> &term) const
 {
 	if (!term) return 0;
-	return term->type == CSGTerm::TYPE_PRIMITIVE ? 1 : 0 + count(term->left) + count(term->right);
+	return term->type == CSGTerm::TYPE_PRIMITIVE ? 0 : 1 + count(term->left) + count(term->right);
 }

src/csgtermnormalizer.h

@@ -6,15 +6,20 @@
 class CSGTermNormalizer
 {
 public:
-	CSGTermNormalizer() {}
+	CSGTermNormalizer(size_t limit) : limit(limit) {}
 	~CSGTermNormalizer() {}
 
-	shared_ptr<class CSGTerm> normalize(const shared_ptr<CSGTerm> &term, size_t limit);
+	shared_ptr<class CSGTerm> normalize(const shared_ptr<CSGTerm> &term);
 
 private:
 	shared_ptr<CSGTerm> normalizePass(shared_ptr<CSGTerm> term) ;
-	bool normalize_tail(shared_ptr<CSGTerm> &term);
+	bool match_and_replace(shared_ptr<CSGTerm> &term);
+	shared_ptr<CSGTerm> collapse_null_terms(const shared_ptr<CSGTerm> &term);
 	unsigned int count(const shared_ptr<CSGTerm> &term) const;
+
+	size_t limit;
+	size_t nodecount;
+	shared_ptr<class CSGTerm> rootnode;
 };
 
 #endif

src/mainwin.cc

@@ -719,9 +719,9 @@ void MainWindow::compileCSG(bool procevents)
 		if (procevents)
 			QApplication::processEvents();
 		
-		CSGTermNormalizer normalizer;
 		size_t normalizelimit = 2 * Preferences::inst()->getValue("advanced/openCSGLimit").toUInt();
-		this->root_norm_term = normalizer.normalize(this->root_raw_term, normalizelimit);
+		CSGTermNormalizer normalizer(normalizelimit);
+		this->root_norm_term = normalizer.normalize(this->root_raw_term);
 		if (this->root_norm_term) {
 			this->root_chain = new CSGChain();
 			this->root_chain->import(this->root_norm_term);
@@ -741,7 +741,7 @@ void MainWindow::compileCSG(bool procevents)
 			
 			highlights_chain = new CSGChain();
 			for (unsigned int i = 0; i < highlight_terms.size(); i++) {
-				highlight_terms[i] = normalizer.normalize(highlight_terms[i], normalizelimit);
+				highlight_terms[i] = normalizer.normalize(highlight_terms[i]);
 				highlights_chain->import(highlight_terms[i]);
 			}
 		}
@@ -754,7 +754,7 @@ void MainWindow::compileCSG(bool procevents)
 			
 			background_chain = new CSGChain();
 			for (unsigned int i = 0; i < background_terms.size(); i++) {
-				background_terms[i] = normalizer.normalize(background_terms[i], normalizelimit);
+				background_terms[i] = normalizer.normalize(background_terms[i]);
 				background_chain->import(background_terms[i]);
 			}
 		}

tests/csgtestcore.cc

@@ -302,8 +302,8 @@ int csgtestcore(int argc, char *argv[], test_type_e test_type)
 	}
 
 	// CSG normalization
-	CSGTermNormalizer normalizer;
+	CSGTermNormalizer normalizer(5000);
-	csgInfo.root_norm_term = normalizer.normalize(root_raw_term, 5000);
+	csgInfo.root_norm_term = normalizer.normalize(root_raw_term);
 	if (csgInfo.root_norm_term) {
 		csgInfo.root_chain = new CSGChain();
 		csgInfo.root_chain->import(csgInfo.root_norm_term);
@@ -319,7 +319,7 @@ int csgtestcore(int argc, char *argv[], test_type_e test_type)
 		
 		csgInfo.highlights_chain = new CSGChain();
 		for (unsigned int i = 0; i < csgInfo.highlight_terms.size(); i++) {
-			csgInfo.highlight_terms[i] = normalizer.normalize(csgInfo.highlight_terms[i], 5000);
+			csgInfo.highlight_terms[i] = normalizer.normalize(csgInfo.highlight_terms[i]);
 			csgInfo.highlights_chain->import(csgInfo.highlight_terms[i]);
 		}
 	}
@@ -329,7 +329,7 @@ int csgtestcore(int argc, char *argv[], test_type_e test_type)
 		
 		csgInfo.background_chain = new CSGChain();
 		for (unsigned int i = 0; i < csgInfo.background_terms.size(); i++) {
-			csgInfo.background_terms[i] = normalizer.normalize(csgInfo.background_terms[i], 5000);
+			csgInfo.background_terms[i] = normalizer.normalize(csgInfo.background_terms[i]);
 			csgInfo.background_chain->import(csgInfo.background_terms[i]);
 		}
 	}