widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

First round of code review done. Not tested yet.

Diff comments:

> 
> === added file 'src/editor/tools/resize_tool.cc'
> --- src/editor/tools/resize_tool.cc	1970-01-01 00:00:00 +0000
> +++ src/editor/tools/resize_tool.cc	2019-04-07 17:11:51 +0000
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (C) 2002-2019 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#include "editor/tools/resize_tool.h"
> +
> +#include "editor/editorinteractive.h"
> +#include "logic/field.h"
> +#include "logic/widelands_geometry.h"
> +
> +int32_t EditorResizeTool::handle_click_impl(const Widelands::World& world,
> +                                                  const Widelands::NodeAndTriangle<>& center,
> +                                                  EditorInteractive& parent,
> +                                                  EditorActionArgs* args,
> +                                                  Widelands::Map* map) {
> +	Widelands::EditorGameBase& egbase = parent.egbase();
> +
> +	args->resized.old_map_size = map->extent();
> +	args->resized.port_spaces.clear();
> +	args->resized.starting_positions.clear();
> +	for (const Widelands::Coords& ps : map->get_port_spaces()) {
> +		args->resized.port_spaces.insert(Widelands::Coords(ps));
> +	}
> +	for (uint8_t i = 1; i <= map->get_nrplayers(); ++i) {
> +		args->resized.starting_positions.push_back(map->get_starting_pos(i));
> +	}
> +
> +	args->resized.deleted_fields = map->resize(egbase, center.node, args->new_map_size.w, args->new_map_size.h);
> +
> +	map->recalc_whole_map(world);
> +	return 0;
> +}
> +
> +int32_t EditorResizeTool::handle_undo_impl(
> +   const Widelands::World& world,
> +   const Widelands::NodeAndTriangle<Widelands::Coords>& center,
> +   EditorInteractive& parent,
> +   EditorActionArgs* args,
> +   Widelands::Map* map) {
> +	Widelands::EditorGameBase& egbase = parent.egbase();
> +
> +	map->resize(egbase, center.node, args->resized.old_map_size.w, args->resized.old_map_size.h);
> +
> +	for (const auto& it : args->resized.deleted_fields) {
> +		const Widelands::FCoords f = map->get_fcoords(it.first);
> +		const Widelands::FieldData data = it.second;
> +
> +		if (Widelands::BaseImmovable* imm = f.field->get_immovable()) {
> +			imm->remove(egbase);
> +		}
> +		for (Widelands::Bob* bob = f.field->get_first_bob(); bob; bob = bob->get_next_bob()) {
> +			bob->remove(egbase);
> +		}
> +
> +		f.field->set_height(data.height);
> +		f.field->set_terrains(data.terrains);
> +		map->initialize_resources(f, data.resources, data.resource_amount);
> +
> +		if (data.immovable != "") {

use !data.immovable.empty()

> +			egbase.create_immovable_with_name(f, data.immovable,
> +					Widelands::MapObjectDescr::OwnerType::kWorld, nullptr, nullptr);
> +		}
> +		for (const std::string& bob : data.bobs) {
> +			egbase.create_critter(f, bob);
> +		}
> +	}
> +
> +	for (const Widelands::Coords& c : args->resized.port_spaces) {
> +		map->set_port_space(world, c, true, true);
> +	}
> +	for (uint8_t i = 1; i <= map->get_nrplayers(); ++i) {
> +		map->set_starting_pos(i, args->resized.starting_positions[i - 1]);
> +	}
> +
> +	map->recalc_whole_map(world);
> +	return 0;
> +}
> +
> +EditorActionArgs EditorResizeTool::format_args_impl(EditorInteractive& parent) {
> +	EditorActionArgs a(parent);
> +	a.new_map_size = Widelands::Extent(width_, height_);
> +	return a;
> +}
> +
> 
> === modified file 'src/editor/ui_menus/tool_menu.cc'
> --- src/editor/ui_menus/tool_menu.cc	2019-02-23 11:00:49 +0000
> +++ src/editor/ui_menus/tool_menu.cc	2019-04-07 17:11:51 +0000
> @@ -65,6 +66,7 @@
>  	ADD_BUTTON("set_port_space", _("Set port space"));
>  	ADD_BUTTON("set_origin", _("Set the position that will have the coordinates (0, 0). This will "
>  	                           "be the top-left corner of a generated minimap."));
> +	ADD_BUTTON("resize", _("Insert or delete rows and columns"));

I think this hint is too technical. How about "Change the map's size"?

>  
>  	set_inner_size(offs.x + (width + spacing) * num_tools, offs.y + (height + spacing));
>  
> @@ -82,7 +84,10 @@
>  		                          5 :
>  		                          &current == &parent.tools()->set_port_space ?
>  		                          6 :
> -		                          &current == &parent.tools()->set_origin ? 7 : 0);
> +		                          &current == &parent.tools()->set_origin ?

Ekk. I know you inherited this code, but I think we should check whether we can have a switch statement here to make things more readable.

> +		                          7 :
> +		                          &current == &parent.tools()->resize ?
> +		                          8 : 0);
>  	}
>  
>  	radioselect_.changed.connect(boost::bind(&EditorToolMenu::changed_to, this));
> 
> === modified file 'src/logic/map.cc'
> --- src/logic/map.cc	2019-03-24 13:16:11 +0000
> +++ src/logic/map.cc	2019-04-07 17:11:51 +0000
> @@ -524,6 +544,153 @@
>  	log("Map origin was shifted by (%d, %d)\n", new_origin.x, new_origin.y);
>  }
>  
> +/* Helper function for resize():
> + * Calculates the coords of 'c' after resizing the map from the given old size to the given new size at 'split'.
> + */
> +static Coords transform_coords(const Coords& c, const Coords& split,
> +		int16_t w_new, int16_t h_new, int16_t w_old, int16_t h_old) {
> +	const int16_t delta_w = w_new - w_old;
> +	const int16_t delta_h = h_new - h_old;
> +	if (c.x < split.x && c.y < split.y) {
> +		// Nothing to shift
> +		Coords result(c);
> +		Map::normalize_coords(result, w_new, h_new);
> +		return result;
> +	} else if ((w_new < w_old && c.x >= split.x && c.x < split.x - delta_w) ||
> +			(h_new < h_old && c.y >= split.y && c.y < split.y - delta_h)) {
> +		// Field removed
> +		return Coords::null();
> +	}
> +	Coords result(c.x, c.y);
> +	if (c.x >= split.x) {
> +		result.x += delta_w;
> +	}
> +	if (c.y >= split.y) {
> +		result.y += delta_h;
> +	}
> +	Map::normalize_coords(result, w_new, h_new);
> +	return result;
> +}
> +
> +/* Change the size of the (already initialized) map to 'w'×'h' by inserting/deleting fields south and east of 'split'.
> + * Returns the data of fields that were deleted during resizing.
> + * This function will notify all players of the change in map size, but not of anything else. This is because
> + * the editor may want to do some post-resize cleanup first, and this function is intended to be used only
> + * by the editor anyway.
> + * You should call recalc_whole_map() afterwards to resolve height differences etc.
> + */
> +std::map<Coords, FieldData> Map::resize(EditorGameBase& egbase, const Coords split, const int32_t w, const int32_t h) {
> +	assert(w > 0);
> +	assert(h > 0);
> +
> +	std::map<Coords, FieldData> deleted;
> +	if (w == width_ && h == height_) {
> +		return deleted;
> +	}
> +
> +	const uint32_t field_size = w * h;
> +	const uint32_t old_field_size = width_ * height_;
> +
> +	std::unique_ptr<Field[]> new_fields(new Field[field_size]);
> +	clear_array(&new_fields, field_size);
> +
> +	// Take care of starting positions and port spaces
> +	for (uint8_t i = get_nrplayers(); i;) {
> +		--i;

for (uint8_t i = get_nrplayers(); i > 0; --i)

> +		if (starting_pos_[i]) {
> +			starting_pos_[i] = transform_coords(starting_pos_[i], split, w, h, width_, height_);
> +		}
> +	}
> +
> +	PortSpacesSet new_port_spaces;
> +	for (Coords it : port_spaces_) {
> +		if (Coords c = transform_coords(it, split, w, h, width_, height_)) {
> +			new_port_spaces.insert(c);
> +		}
> +	}
> +	port_spaces_ = new_port_spaces;
> +
> +	Field::Terrains default_terrains;
> +	default_terrains.r = 0;
> +	default_terrains.d = 0;
> +
> +	std::unique_ptr<bool[]> preserved_coords(new bool[old_field_size]);
> +	memset(preserved_coords.get(), false, sizeof(bool) * old_field_size);

If you replaced Field in clear_array with a template parameter and passed in the default value, we could use it here too.

> +
> +	for (int16_t x = 0; x < w; ++x) {
> +		for (int16_t y = 0; y < h; ++y) {
> +			Coords c_new = Coords(x, y);
> +			if (Coords c_old = transform_coords(c_new, split, width_, height_, w, h)) {
> +				bool& entry = preserved_coords[get_index(c_old, width_)];
> +				if (!entry) {
> +					// Copy existing field
> +					entry = true;
> +					new_fields[get_index(c_new, w)] = operator[](c_old);
> +					continue;
> +				}
> +			}
> +			// Init new field
> +			Field& field = new_fields[get_index(c_new, w)];
> +			field.set_height(10);
> +			field.set_terrains(default_terrains);
> +		}
> +	}
> +
> +	for (int16_t x = 0; x < width_; ++x) {
> +		for (int16_t y = 0; y < height_; ++y) {
> +			Coords c(x, y);
> +			if (!preserved_coords[get_index(c, width_)]) {

Can you do what you're doing here in the loop above, so you don't have to loop twice?

> +				// Save the data of fields that will be deleted
> +				Field& field = operator[](c);
> +				deleted.emplace(c, FieldData(field));

It' better to use deleted.insert(std::make_pair(c, FieldData(field)));
I don't remember why though ;)

> +				// ...and now we delete stuff that needs removing when the field is destroyed
> +				if (BaseImmovable* imm = field.get_immovable()) {
> +					imm->remove(egbase);
> +				}
> +				while (Bob* bob = field.get_first_bob()) {
> +					bob->remove(egbase);
> +				}
> +			}
> +		}
> +	}
> +
> +	// Replace all fields
> +	fields_.reset(new Field[field_size]);
> +	clear_array(&fields_, field_size);
> +	for (size_t ind = 0; ind < field_size; ind++) {

It's a C++ convention to use ++ind rather than ind++.

> +		fields_[ind] = new_fields[ind];
> +	}
> +	log("Resized map from (%d, %d) to (%u, %u) at (%d, %d)\n", width_, height_, w, h, split.x, split.y);
> +	width_ = w;
> +	height_ = h;
> +
> +	// Inform immovables and bobs about their new position
> +	for (int16_t x = 0; x < w; ++x) {

Using Field& operator[](MapIndex) to get at the fields is probably faster

> +		for (int16_t y = 0; y < h; ++y) {
> +			FCoords f = get_fcoords(Coords(x, y));
> +			if (upcast(Immovable, immovable, f.field->get_immovable())) {
> +				immovable->position_ = f;
> +			}
> +			// Ensuring that all bob iterators are changed correctly is a bit hacky, but the more obvious
> +			// solution of doing it like in set_origin() is highly problematic here, or so ASan tells me
> +			std::vector<Bob*> bobs;
> +			for (Bob* bob = f.field->get_first_bob(); bob; bob = bob->get_next_bob()) {
> +				bobs.push_back(bob);
> +			}
> +			f.field->bobs = nullptr;
> +			for (Bob* bob : bobs) {
> +				bob->position_.field = nullptr;
> +				bob->linknext_ = nullptr;
> +				bob->linkpprev_ = nullptr;
> +				bob->set_position(egbase, f);
> +			}
> +		}
> +	}
> +
> +	egbase.allocate_player_maps();
> +	return deleted;
> +}
> +
>  /*
>  ===============
>  Set the size of the map. This should only happen once during initial load.
> 
> === modified file 'src/logic/map.h'
> --- src/logic/map.h	2019-03-24 13:16:11 +0000
> +++ src/logic/map.h	2019-04-07 17:11:51 +0000
> @@ -588,14 +604,18 @@
>  }
>  
>  inline void Map::normalize_coords(Coords& c) const {
> +	normalize_coords(c, width_, height_);
> +}
> +
> +inline void Map::normalize_coords(Coords& c, int16_t w, int16_t h) {
>  	while (c.x < 0)
> -		c.x += width_;
> -	while (c.x >= width_)
> -		c.x -= width_;
> +		c.x += w;
> +	while (c.x >= w)

While you're touching this anyway, please add {}

> +		c.x -= w;
>  	while (c.y < 0)
> -		c.y += height_;
> -	while (c.y >= height_)
> -		c.y -= height_;
> +		c.y += h;
> +	while (c.y >= h)
> +		c.y -= h;
>  }
>  
>  /**


-- 
https://code.launchpad.net/~widelands-dev/widelands/editor-resize-map/+merge/365638
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/editor-resize-map into lp:widelands.