widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve

Code LGTM - just add the optional braces as I commented below, and it's good to go.

Thanks for the fix! :)

Diff comments:

> === modified file 'src/editor/tools/set_origin_tool.cc'
> --- src/editor/tools/set_origin_tool.cc	2017-08-17 15:34:45 +0000
> +++ src/editor/tools/set_origin_tool.cc	2017-12-19 18:24:38 +0000
> @@ -41,7 +41,14 @@
>                                        EditorActionArgs* /* args */,
>                                        Widelands::Map* map) {
>  	Widelands::Coords nc(
> -	   map->get_width() - 1 - center.node.x, map->get_height() - 1 - center.node.y);
> +	   map->get_width() - center.node.x, map->get_height() - center.node.y);
> +
> +	// Because of the triangle design of map, y is changed by an odd number.
> +	// The x must be syncronized with the y when coordinate pair is applied
> +	// and also when undoing an action like here.
> +	if ((nc.y % 2) != 0)

Please add {} - C++ doesn't require them, but they can cause nasty bugs when a mistake is made.

> +		nc.x = nc.x - 1;
> +	map->normalize_coords(nc);
>  	map->set_origin(nc);
>  	eia.map_changed(EditorInteractive::MapWas::kGloballyMutated);
>  	eia.map_view()->scroll_to_field(Widelands::Coords(0, 0), MapView::Transition::Jump);
> 
> === modified file 'src/logic/map.cc'
> --- src/logic/map.cc	2017-12-18 10:42:40 +0000
> +++ src/logic/map.cc	2017-12-19 18:24:38 +0000
> @@ -359,7 +359,8 @@
>  		}
>  	}
>  	// Now that we restructured the fields, we just overwrite the old order
> -	fields_.reset(new_field_order.release());
> +	for (decltype(width_) ind = 0; ind < width_*height_; ind++)

Please add {} - C++ doesn't require them, but they can cause nasty bugs when a mistake is made.

> +		fields_[ind] = new_field_order[ind];
>  
>  	//  Inform immovables and bobs about their new coordinates.
>  	for (FCoords c(Coords(0, 0), fields_.get()); c.y < height_; ++c.y)


-- 
https://code.launchpad.net/~widelands-dev/widelands/fix-bug-1735980-related-problems/+merge/335403
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fix-bug-1735980-related-problems.