NameIdMap, error handling clean-up

README.md

@@ -1,5 +1,7 @@
 # MapEditr
 
+TODO: Add a license.
+
 MapEditr is a command-line tool for relatively fast manipulation of Minetest
 worlds. It can replace nodes, fill areas, combine parts of different worlds,
 and much more.

src/block_utils.rs

@@ -14,6 +14,9 @@ fn block_parts_valid(a: &Area, b: &Area) -> bool {
 }
 
 
+/// Copy an area of nodes from one mapblock to another.
+///
+/// Will not remove duplicate/unused name IDs.
 pub fn merge_blocks(
 	src_block: &MapBlock,
 	dst_block: &mut MapBlock,
@@ -24,14 +27,14 @@ pub fn merge_blocks(
 
 	let src_nd = src_block.node_data.get_ref();
 	let dst_nd = dst_block.node_data.get_mut();
-
 	let offset = dst_area.min - src_area.min;
 	// Warning: diff can be negative!
 	let diff = offset.x + offset.y * 16 + offset.z * 256;
 
+	// Copy name-ID mappings
 	let nimap_diff = dst_block.nimap.get_max_id().unwrap() + 1;
-	for (&id, name) in &src_block.nimap.map {
+	for (id, name) in &src_block.nimap.0 {
-		dst_block.nimap.insert(id + nimap_diff, name)
+		dst_block.nimap.0.insert(id + nimap_diff, name.to_vec());
 	}
 
 	// Copy node IDs
@@ -65,6 +68,7 @@ pub fn merge_blocks(
 }
 
 
+/// Copy an area of node metadata from one mapblock to another.
 pub fn merge_metadata(
 	src_meta: &NodeMetadataList,
 	dst_meta: &mut NodeMetadataList,
@@ -119,7 +123,7 @@ pub fn clean_name_id_map(block: &mut MapBlock) {
 			continue;
 		}
 
-		let name = &block.nimap.map[&(id as u16)];
+		let name = &block.nimap.0[&(id as u16)];
 		if let Some(first_id) = new_nimap.iter().position(|(_, v)| v == name) {
 			// Name is already in the map; map old, duplicate ID to the
 			// existing ID.
@@ -131,7 +135,7 @@ pub fn clean_name_id_map(block: &mut MapBlock) {
 			map[id] = new_nimap.len() as u16 - 1;
 		}
 	}
-	block.nimap.map = new_nimap;
+	block.nimap.0 = new_nimap;
 
 	// Re-assign node IDs.
 	for id in &mut nd.nodes {

src/commands/clone.rs

@@ -60,10 +60,10 @@ fn clone(inst: &mut InstBundle) {
 			opt_unwrap_or!(
 				get_cached(&mut inst.db, &mut block_cache, dst_key),
 				continue
-			).and_then(|b|
+			).and_then(|b| -> Result<_, MapBlockError> {
-				NodeMetadataList::deserialize(b.metadata.get_ref())
+				let m = NodeMetadataList::deserialize(b.metadata.get_ref())?;
-					.map(|m| (b, m))
+				Ok((b, m))
-			),
+			}),
 			{ inst.status.inc_failed(); continue; }
 		);
 
@@ -79,12 +79,13 @@ fn clone(inst: &mut InstBundle) {
 			}
 			let src_key = src_pos.to_block_key();
 			let (src_block, src_meta) = opt_unwrap_or!(
-				get_cached(&mut inst.db, &mut block_cache, src_key)
+				|| -> Option<_> {
-					.map(Result::ok).flatten()
+					let b = get_cached(
-					.and_then(|b|
+						&mut inst.db, &mut block_cache, src_key)?.ok()?;
-						NodeMetadataList::deserialize(b.metadata.get_ref())
+					let m = NodeMetadataList::deserialize(b.metadata.get_ref())
-							.ok().map(|m| (b, m))
+						.ok()?;
-					),
+					Some((b, m))
+				}(),
 				continue
 			);
 

src/commands/fill.rs

@@ -3,7 +3,7 @@ use super::Command;
 use crate::unwrap_or;
 use crate::spatial::{Vec3, Area, area_rel_block_overlap, area_contains_block};
 use crate::instance::{ArgType, InstBundle};
-use crate::map_block::{MapBlock};
+use crate::map_block::MapBlock;
 use crate::block_utils::clean_name_id_map;
 use crate::utils::{query_keys, to_bytes, fmt_big_num};
 
@@ -47,14 +47,14 @@ fn fill(inst: &mut InstBundle) {
 			for x in &mut nd.nodes {
 				*x = 0;
 			}
-			block.nimap.map.clear();
+			block.nimap.0.clear();
-			block.nimap.insert(0, &node);
+			block.nimap.0.insert(0, node.to_vec());
 			count += nd.nodes.len() as u64;
 		} else {
 			let slice = area_rel_block_overlap(&area, pos).unwrap();
 			let fill_id = block.nimap.get_id(&node).unwrap_or_else(|| {
 				let next = block.nimap.get_max_id().unwrap() + 1;
-				block.nimap.insert(next, &node);
+				block.nimap.0.insert(next, node.to_vec());
 				next
 			});
 			fill_area(&mut block, slice, fill_id);

src/commands/overlay.rs

@@ -1,12 +1,12 @@
 use super::{Command, BLOCK_CACHE_SIZE};
 
-use crate::opt_unwrap_or;
+use crate::{unwrap_or, opt_unwrap_or};
 use crate::spatial::{Vec3, Area, area_rel_block_overlap,
 	area_abs_block_overlap, area_contains_block, area_touches_block};
 use crate::instance::{ArgType, InstArgs, InstBundle};
 use crate::map_database::MapDatabase;
-use crate::map_block::{MapBlock, MapBlockError, NodeMetadataList,
+use crate::map_block::{MapBlock, NodeMetadataList, is_valid_generated,
-	is_valid_generated};
+	MapBlockError};
 use crate::block_utils::{merge_blocks, merge_metadata, clean_name_id_map};
 use crate::utils::{query_keys, CacheMap};
 
@@ -29,11 +29,12 @@ fn verify_args(args: &InstArgs) -> anyhow::Result<()> {
 /// - Area + Invert
 #[inline]
 fn overlay_no_offset(inst: &mut InstBundle) {
-	let mut idb = inst.idb.as_mut().unwrap();
+	let db = &mut inst.db;
+	let idb = inst.idb.as_mut().unwrap();
 	let invert = inst.args.invert;
 
 	// Get keys from input database.
-	let keys = query_keys(&mut idb, &inst.status,
+	let keys = query_keys(idb, &inst.status,
 		&[], inst.args.area, invert, true);
 	inst.status.begin_editing();
 
@@ -48,50 +49,54 @@ fn overlay_no_offset(inst: &mut InstBundle) {
 			{ // If possible, copy whole map block.
 				let data = idb.get_block(key).unwrap();
 				if is_valid_generated(&data) {
-					inst.db.set_block(key, &data).unwrap();
+					db.set_block(key, &data).unwrap();
 				}
 			} else { // Copy part of map block
-				let dst_data = match inst.db.get_block(key) {
+				let res = || -> Result<(), MapBlockError> {
-					Ok(d) => if is_valid_generated(&d) {
+					let dst_data = opt_unwrap_or!(
-						d
+						db.get_block(key).ok()
+							.filter(|d| is_valid_generated(&d)),
+						return Ok(()));
+					let src_data = idb.get_block(key).unwrap();
+
+					let mut src_block = MapBlock::deserialize(&src_data)?;
+					let mut dst_block = MapBlock::deserialize(&dst_data)?;
+					let mut src_meta = NodeMetadataList::deserialize(
+						&src_block.metadata.get_ref())?;
+					let mut dst_meta = NodeMetadataList::deserialize(
+						&dst_block.metadata.get_ref())?;
+
+					let block_part = area_rel_block_overlap(&area, pos)
+						.unwrap();
+					if invert {
+						// For inverted selections, reverse the order of the
+						// overlay operations.
+						merge_blocks(&dst_block, &mut src_block,
+							block_part, block_part);
+						merge_metadata(&dst_meta, &mut src_meta,
+							block_part, block_part);
+						clean_name_id_map(&mut src_block);
+						db.set_block(key, &src_block.serialize()).unwrap();
 					} else {
-						continue;
+						merge_blocks(&src_block, &mut dst_block,
-					},
+							block_part, block_part);
-					Err(_) => continue
+						merge_metadata(&src_meta, &mut dst_meta,
-				};
+							block_part, block_part);
-				let src_data = idb.get_block(key).unwrap();
+						clean_name_id_map(&mut dst_block);
+						db.set_block(key, &dst_block.serialize()).unwrap();
+					}
+					Ok(())
+				}();
 
-				let mut src_block = MapBlock::deserialize(&src_data).unwrap();
+				if res.is_err() {
-				let mut dst_block = MapBlock::deserialize(&dst_data).unwrap();
+					inst.status.inc_failed()
-				let mut src_meta = NodeMetadataList::deserialize(
-					&src_block.metadata.get_ref()).unwrap();
-				let mut dst_meta = NodeMetadataList::deserialize(
-					&dst_block.metadata.get_ref()).unwrap();
-
-				let block_part = area_rel_block_overlap(&area, pos).unwrap();
-				if invert {
-					// For inverted selections, reverse the order of the
-					// overlay operations.
-					merge_blocks(&dst_block, &mut src_block,
-						block_part, block_part);
-					merge_metadata(&dst_meta, &mut src_meta,
-						block_part, block_part);
-					clean_name_id_map(&mut src_block);
-					inst.db.set_block(key, &src_block.serialize()).unwrap();
-				} else {
-					merge_blocks(&src_block, &mut dst_block,
-						block_part, block_part);
-					merge_metadata(&src_meta, &mut dst_meta,
-						block_part, block_part);
-					clean_name_id_map(&mut dst_block);
-					inst.db.set_block(key, &dst_block.serialize()).unwrap();
 				}
 			}
 		} else {
 			// No area; copy whole map block.
 			let data = idb.get_block(key).unwrap();
 			if is_valid_generated(&data) {
-				inst.db.set_block(key, &data).unwrap();
+				db.set_block(key, &data).unwrap();
 			}
 		}
 	}
@@ -100,19 +105,17 @@ fn overlay_no_offset(inst: &mut InstBundle) {
 }
 
 
-type BlockResult = Option<Result<MapBlock, MapBlockError>>;
-
 fn get_cached(
 	db: &mut MapDatabase,
-	cache: &mut CacheMap<i64, BlockResult>,
+	cache: &mut CacheMap<i64, Option<MapBlock>>,
 	key: i64
-) -> BlockResult {
+) -> Option<MapBlock> {
 	match cache.get(&key) {
 		Some(data) => data.clone(),
 		None => {
 			let block = db.get_block(key).ok()
 				.filter(|d| is_valid_generated(d))
-				.map(|d| MapBlock::deserialize(&d));
+				.and_then(|d| MapBlock::deserialize(&d).ok());
 			cache.insert(key, block.clone());
 			block
 		}
@@ -143,11 +146,12 @@ fn overlay_with_offset(inst: &mut InstBundle) {
 			inst.db.get_block(dst_key).ok().filter(|d| is_valid_generated(d)),
 			continue
 		);
-		let (mut dst_block, mut dst_meta) = opt_unwrap_or!(
+		let (mut dst_block, mut dst_meta) = unwrap_or!(
-			MapBlock::deserialize(&dst_data).ok().and_then(|b|
+			|| -> Result<_, MapBlockError> {
-				NodeMetadataList::deserialize(b.metadata.get_ref())
+				let b = MapBlock::deserialize(&dst_data)?;
-					.ok().map(|m| (b, m))
+				let m = NodeMetadataList::deserialize(b.metadata.get_ref())?;
-			),
+				Ok((b, m))
+			}(),
 			{ inst.status.inc_failed(); continue; }
 		);
 
@@ -164,12 +168,12 @@ fn overlay_with_offset(inst: &mut InstBundle) {
 			}
 			let src_key = src_pos.to_block_key();
 			let (src_block, src_meta) = opt_unwrap_or!(
-				get_cached(idb, &mut src_block_cache, src_key)
+				|| -> Option<_> {
-					.map(Result::ok).flatten()
+					let b = get_cached(idb, &mut src_block_cache, src_key)?;
-					.and_then(|b|
+					let m = NodeMetadataList::deserialize(b.metadata.get_ref())
-						NodeMetadataList::deserialize(b.metadata.get_ref())
+						.ok()?;
-							.ok().map(|m| (b, m))
+					Some((b, m))
-					),
+				}(),
 				continue
 			);
 

src/commands/replace_nodes.rs

@@ -62,7 +62,7 @@ fn do_replace(
 
 		// Replacement node not yet in name-ID map; insert it.
 		if new_replace_id && new_node_present {
-			block.nimap.insert(replace_id, new_node);
+			block.nimap.0.insert(replace_id, new_node.to_vec());
 		}
 
 		// Search node was completely eliminated; shift IDs down.
@@ -72,7 +72,7 @@ fn do_replace(
 					nd.nodes[i] -= 1;
 				}
 			}
-			block.nimap.remove(search_id);
+			block.nimap.remove_shift(search_id);
 		}
 	}
 	// Replace nodes in whole map block.
@@ -81,7 +81,7 @@ fn do_replace(
 		if let Some(mut replace_id) = block.nimap.get_id(new_node) {
 			let _t = tk.get_timer("replace (non-unique replacement)");
 			// Delete unused ID from name-ID map and shift IDs down.
-			block.nimap.remove(search_id);
+			block.nimap.remove_shift(search_id);
 			// Shift replacement ID, if necessary.
 			replace_id -= (replace_id > search_id) as u16;
 
@@ -104,7 +104,7 @@ fn do_replace(
 			for id in &nd.nodes {
 				count += (*id == search_id) as u64;
 			}
-			block.nimap.insert(search_id, new_node);
+			block.nimap.0.insert(search_id, new_node.to_vec());
 		}
 	}
 	count

src/commands/set_param2.rs

@@ -2,7 +2,7 @@ use super::Command;
 
 use crate::spatial::{Vec3, Area, area_rel_block_overlap, area_contains_block};
 use crate::instance::{ArgType, InstArgs, InstBundle};
-use crate::map_block::{MapBlock};
+use crate::map_block::MapBlock;
 use crate::utils::{query_keys, to_bytes, to_slice, fmt_big_num};
 
 

src/map_block/map_block.rs

@@ -12,6 +12,8 @@ pub fn is_valid_generated(data: &[u8]) -> bool {
 }
 
 
+// TODO: Allocation limit for everything to prevent crashes with bad data.
+// TODO: Make sure all data structures are the best choice.
 #[derive(Debug, Clone)]
 pub struct MapBlock {
 	pub version: u8,

src/map_block/name_id_map.rs

@@ -4,12 +4,10 @@ use super::*;
 
 
 /// Maps 16-bit node IDs to actual node names.
+///
 /// Relevant Minetest source file: /src/nameidmapping.cpp
 #[derive(Debug, Clone)]
-pub struct NameIdMap {
+pub struct NameIdMap(pub BTreeMap<u16, Vec<u8>>);
-	// Use a BTreeMap instead of a HashMap to preserve the order of IDs.
-	pub map: BTreeMap<u16, Vec<u8>>,
-}
 
 impl NameIdMap {
 	pub fn deserialize(data: &mut Cursor<&[u8]>)
@@ -29,14 +27,14 @@ impl NameIdMap {
 			map.insert(id, name);
 		}
 
-		Ok(Self {map})
+		Ok(Self(map))
 	}
 
 	pub fn serialize(&self, out: &mut Cursor<Vec<u8>>) {
 		out.write_u8(0).unwrap();
-		out.write_u16::<BigEndian>(self.map.len() as u16).unwrap();
+		out.write_u16::<BigEndian>(self.0.len() as u16).unwrap();
 
-		for (id, name) in &self.map {
+		for (id, name) in &self.0 {
 			out.write_u16::<BigEndian>(*id).unwrap();
 			write_string16(out, name);
 		}
@@ -44,29 +42,24 @@ impl NameIdMap {
 
 	#[inline]
 	pub fn get_id(&self, name: &[u8]) -> Option<u16> {
-		self.map.iter().find_map(|(&k, v)|
+		self.0.iter().find_map(|(&k, v)|
 			if v.as_slice() == name { Some(k) } else { None }
 		)
 	}
 
 	#[inline]
 	pub fn get_max_id(&self) -> Option<u16> {
-		self.map.iter().next_back().map(|k| *(k.0))
+		self.0.iter().next_back().map(|(&k, _)| k)
-	}
-
-	#[inline]
-	pub fn insert(&mut self, id: u16, name: &[u8]) {
-		self.map.insert(id, name.to_owned());
 	}
 
 	/// Remove the name at a given ID and shift down values above it.
-	pub fn remove(&mut self, id: u16) {
+	pub fn remove_shift(&mut self, id: u16) {
-		self.map.remove(&id);
+		self.0.remove(&id);
 		let mut next_id = id + 1;
 
-		while self.map.contains_key(&next_id) {
+		while self.0.contains_key(&next_id) {
-			let name = self.map.remove(&next_id).unwrap();
+			let name = self.0.remove(&next_id).unwrap();
-			self.map.insert(next_id - 1, name);
+			self.0.insert(next_id - 1, name);
 			next_id += 1;
 		}
 	}