From 21ee17db5853fe5d103a90c0d2a6331ad5a558ec Mon Sep 17 00:00:00 2001 From: random-geek <35757396+random-geek@users.noreply.github.com> Date: Sat, 6 Mar 2021 15:01:46 -0800 Subject: [PATCH] NameIdMap, error handling clean-up --- README.md | 2 + src/block_utils.rs | 14 +++-- src/commands/clone.rs | 21 ++++--- src/commands/fill.rs | 8 +-- src/commands/overlay.rs | 114 ++++++++++++++++++---------------- src/commands/replace_nodes.rs | 8 +-- src/commands/set_param2.rs | 2 +- src/map_block/map_block.rs | 2 + src/map_block/name_id_map.rs | 31 ++++----- 9 files changed, 104 insertions(+), 98 deletions(-) diff --git a/README.md b/README.md index fbea2c9..16cd235 100644 --- a/README.md +++ b/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. diff --git a/src/block_utils.rs b/src/block_utils.rs index e999f25..20879cd 100644 --- a/src/block_utils.rs +++ b/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 { - dst_block.nimap.insert(id + nimap_diff, name) + for (id, name) in &src_block.nimap.0 { + 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 { diff --git a/src/commands/clone.rs b/src/commands/clone.rs index f6b69f4..75045f1 100644 --- a/src/commands/clone.rs +++ b/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| - NodeMetadataList::deserialize(b.metadata.get_ref()) - .map(|m| (b, m)) - ), + ).and_then(|b| -> Result<_, MapBlockError> { + let m = NodeMetadataList::deserialize(b.metadata.get_ref())?; + 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) - .map(Result::ok).flatten() - .and_then(|b| - NodeMetadataList::deserialize(b.metadata.get_ref()) - .ok().map(|m| (b, m)) - ), + || -> Option<_> { + let b = get_cached( + &mut inst.db, &mut block_cache, src_key)?.ok()?; + let m = NodeMetadataList::deserialize(b.metadata.get_ref()) + .ok()?; + Some((b, m)) + }(), continue ); diff --git a/src/commands/fill.rs b/src/commands/fill.rs index 43cb7ae..57a0ba9 100644 --- a/src/commands/fill.rs +++ b/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.insert(0, &node); + block.nimap.0.clear(); + 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); diff --git a/src/commands/overlay.rs b/src/commands/overlay.rs index dd33392..9872bb0 100644 --- a/src/commands/overlay.rs +++ b/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, - is_valid_generated}; +use crate::map_block::{MapBlock, NodeMetadataList, 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) { - Ok(d) => if is_valid_generated(&d) { - d + let res = || -> Result<(), MapBlockError> { + let dst_data = opt_unwrap_or!( + 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; - }, - Err(_) => continue - }; - let src_data = idb.get_block(key).unwrap(); + 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); + db.set_block(key, &dst_block.serialize()).unwrap(); + } + Ok(()) + }(); - let mut src_block = MapBlock::deserialize(&src_data).unwrap(); - let mut dst_block = MapBlock::deserialize(&dst_data).unwrap(); - 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(); + if res.is_err() { + inst.status.inc_failed() } } } 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>; - fn get_cached( db: &mut MapDatabase, - cache: &mut CacheMap, + cache: &mut CacheMap>, key: i64 -) -> BlockResult { +) -> Option { 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!( - MapBlock::deserialize(&dst_data).ok().and_then(|b| - NodeMetadataList::deserialize(b.metadata.get_ref()) - .ok().map(|m| (b, m)) - ), + let (mut dst_block, mut dst_meta) = unwrap_or!( + || -> Result<_, MapBlockError> { + let b = MapBlock::deserialize(&dst_data)?; + 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) - .map(Result::ok).flatten() - .and_then(|b| - NodeMetadataList::deserialize(b.metadata.get_ref()) - .ok().map(|m| (b, m)) - ), + || -> Option<_> { + let b = get_cached(idb, &mut src_block_cache, src_key)?; + let m = NodeMetadataList::deserialize(b.metadata.get_ref()) + .ok()?; + Some((b, m)) + }(), continue ); diff --git a/src/commands/replace_nodes.rs b/src/commands/replace_nodes.rs index 8a4d6d6..976b864 100644 --- a/src/commands/replace_nodes.rs +++ b/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 diff --git a/src/commands/set_param2.rs b/src/commands/set_param2.rs index c92ea45..0ab1c3f 100644 --- a/src/commands/set_param2.rs +++ b/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}; diff --git a/src/map_block/map_block.rs b/src/map_block/map_block.rs index 8f2a3b5..2fd0d16 100644 --- a/src/map_block/map_block.rs +++ b/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, diff --git a/src/map_block/name_id_map.rs b/src/map_block/name_id_map.rs index a4e4b02..57ddf9b 100644 --- a/src/map_block/name_id_map.rs +++ b/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 { - // Use a BTreeMap instead of a HashMap to preserve the order of IDs. - pub map: BTreeMap>, -} +pub struct NameIdMap(pub BTreeMap>); 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>) { out.write_u8(0).unwrap(); - out.write_u16::(self.map.len() as u16).unwrap(); + out.write_u16::(self.0.len() as u16).unwrap(); - for (id, name) in &self.map { + for (id, name) in &self.0 { out.write_u16::(*id).unwrap(); write_string16(out, name); } @@ -44,29 +42,24 @@ impl NameIdMap { #[inline] pub fn get_id(&self, name: &[u8]) -> Option { - 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 { - self.map.iter().next_back().map(|k| *(k.0)) - } - - #[inline] - pub fn insert(&mut self, id: u16, name: &[u8]) { - self.map.insert(id, name.to_owned()); + self.0.iter().next_back().map(|(&k, _)| k) } /// Remove the name at a given ID and shift down values above it. - pub fn remove(&mut self, id: u16) { - self.map.remove(&id); + pub fn remove_shift(&mut self, id: u16) { + self.0.remove(&id); let mut next_id = id + 1; - while self.map.contains_key(&next_id) { - let name = self.map.remove(&next_id).unwrap(); - self.map.insert(next_id - 1, name); + while self.0.contains_key(&next_id) { + let name = self.0.remove(&next_id).unwrap(); + self.0.insert(next_id - 1, name); next_id += 1; } }