Clear old item groups when they are overridden. (#8753)

This fixes overridden items keeping their old groups in the group to
items mapping even after their groups have been changed in lua.
It also prevents a more widespread issue where overriding an item
will add its content ID *twice* to the mapping, resulting in odd
behaviour in features such as ABMs.

2 files changed, 36 insertions, 14 deletions

diff --git a/src/nodedef.cpp b/src/nodedef.cpp
index 2ffdf2fc2..977a4533d 100644
--- a/src/nodedef.cpp
+++ b/src/nodedef.cpp
@@ -1202,6 +1202,26 @@ inline void NodeDefManager::fixSelectionBoxIntUnion()
 }
 
 
+void NodeDefManager::eraseIdFromGroups(content_t id)
+{
+	// For all groups in m_group_to_items...
+	for (auto iter_groups = m_group_to_items.begin();
+			iter_groups != m_group_to_items.end();) {
+		// Get the group items vector.
+		std::vector<content_t> &items = iter_groups->second;
+
+		// Remove any occurence of the id in the group items vector.
+		items.erase(std::remove(items.begin(), items.end(), id), items.end());
+
+		// If group is empty, erase its vector from the map.
+		if (items.empty())
+			iter_groups = m_group_to_items.erase(iter_groups);
+		else
+			++iter_groups;
+	}
+}
+
+
 // IWritableNodeDefManager
 content_t NodeDefManager::set(const std::string &name, const ContentFeatures &def)
 {
@@ -1222,19 +1242,24 @@ content_t NodeDefManager::set(const std::string &name, const ContentFeatures &de
 		assert(id != CONTENT_IGNORE);
 		addNameIdMapping(id, name);
 	}
+
+	// If there is already ContentFeatures registered for this id, clear old groups
+	if (id < m_content_features.size())
+		eraseIdFromGroups(id);
+
 	m_content_features[id] = def;
 	verbosestream << "NodeDefManager: registering content id \"" << id
 		<< "\": name=\"" << def.name << "\""<<std::endl;
 
 	getNodeBoxUnion(def.selection_box, def, &m_selection_box_union);
 	fixSelectionBoxIntUnion();
+
 	// Add this content to the list of all groups it belongs to
-	// FIXME: This should remove a node from groups it no longer
-	// belongs to when a node is re-registered
 	for (const auto &group : def.groups) {
 		const std::string &group_name = group.first;
 		m_group_to_items[group_name].push_back(id);
 	}
+
 	return id;
 }
 
@@ -1260,18 +1285,7 @@ void NodeDefManager::removeNode(const std::string &name)
 		m_name_id_mapping_with_aliases.erase(name);
 	}
 
-	// Erase node content from all groups it belongs to
-	for (std::unordered_map<std::string, std::vector<content_t>>::iterator iter_groups =
-			m_group_to_items.begin(); iter_groups != m_group_to_items.end();) {
-		std::vector<content_t> &items = iter_groups->second;
-		items.erase(std::remove(items.begin(), items.end(), id), items.end());
-
-		// Check if group is empty
-		if (items.empty())
-			m_group_to_items.erase(iter_groups++);
-		else
-			++iter_groups;
-	}
+	eraseIdFromGroups(id);
 }
 
 
diff --git a/src/nodedef.h b/src/nodedef.h
index 60d91f8d9..1a12aae93 100644
--- a/src/nodedef.h
+++ b/src/nodedef.h
@@ -669,6 +669,14 @@ private:
 	void addNameIdMapping(content_t i, std::string name);
 
 	/*!
+	 * Removes a content ID from all groups.
+	 * Erases content IDs from vectors in \ref m_group_to_items and
+	 * removes empty vectors.
+	 * @param id Content ID
+	 */
+	void eraseIdFromGroups(content_t id);
+
+	/*!
 	 * Recalculates m_selection_box_int_union based on
 	 * m_selection_box_union.
 	 */