widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Resubmit

Should be all good now. I did some mild refactorings during bug tracking too. PTAL.

Diff comments:

> 
> === modified file 'src/editor/tools/editor_set_resources_tool.cc'
> --- src/editor/tools/editor_set_resources_tool.cc	2016-01-04 20:50:19 +0000
> +++ src/editor/tools/editor_set_resources_tool.cc	2016-01-07 09:30:32 +0000
> @@ -102,31 +98,28 @@
>  
>  void EditorSetResourcesTool::set_res_and_overlay(const Widelands::World& world,
>  											int32_t amount, uint8_t new_res,
> -											Widelands::MapRegion<Widelands::Area<Widelands::FCoords> >* mr,
> -											EditorActionArgs* args,
> +											const Widelands::FCoords& fcoords,

urgs... we should really switch to space only for indentation and do away with tabs.

>  											Widelands::Map* map) {
> -	int32_t old_res = mr->location().field->get_resources();
> +	int32_t old_res = fcoords.field->get_resources();
>  
>  	//  Ok, we're doing something. First remove the current overlays.
>  	if (old_res != Widelands::kNoResource) {
>  		std::string str = world.get_resource(old_res)->get_editor_pic(
> -				mr->location().field->get_resources_amount());
> +				fcoords.field->get_resources_amount());
>  		const Image* pic = g_gr->images().get(str);
> -		map->overlay_manager().remove_overlay(mr->location(), pic);
> +		map->overlay_manager().remove_overlay(fcoords, pic);
>  	}
>  
>  	if (!amount) {
> -		mr->location().field->set_resources(Widelands::kNoResource, 0);
> -		mr->location().field->set_initial_res_amount(0);
> +		fcoords.field->set_resources(Widelands::kNoResource, 0);
> +		fcoords.field->set_initial_res_amount(0);
>  	} else {
> -		mr->location().field->set_resources(new_res, amount);
> -		mr->location().field->set_initial_res_amount(amount);
> +		fcoords.field->set_resources(new_res, amount);
> +		fcoords.field->set_initial_res_amount(amount);
>  		//  set new overlay
>  		std::string str = world.get_resource(new_res)->get_editor_pic(amount);
>  		const Image* pic = g_gr->images().get(str);
> -		map->overlay_manager().register_overlay(mr->location(), pic, 4);
> -		map->recalc_for_field_area(
> -				world, Widelands::Area<Widelands::FCoords>(mr->location(), 0));
> +		map->overlay_manager().register_overlay(fcoords, pic, 0);
> +		map->recalc_for_field_area(world, Widelands::Area<Widelands::FCoords>(fcoords, 0));
>  	}
>  }
> -
> 
> === modified file 'src/editor/ui_menus/editor_tool_change_resources_options_menu.cc'
> --- src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-01-05 10:07:13 +0000
> +++ src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-01-07 09:30:32 +0000
> @@ -222,8 +222,12 @@
>  	Widelands::EditorGameBase& egbase = eia().egbase();
>  	Widelands::Map & map = egbase.map();
>  	map.overlay_manager().register_overlay_callback_function(
> -			[&] (const Widelands::TCoords<Widelands::FCoords>& coords)
> -			{return map.is_resource_valid(egbase.world(), coords, resIx);});
> +		[resIx, &map, &egbase](const Widelands::TCoords<Widelands::FCoords>& coords) -> uint32_t {

Janosch, fyi: It took me forever to track down this bug and I also see it very frequently in my professional live and it is always hard to find, so I thought I poke you about it, so it can be avoided in the future. 

You captured all used variables by reference here in this lambda using [&]. The problem was that resIx was also captured by reference, i.e. a pointer to it was saved. It is only a local variable in the outer function though and therefore the pointer becomes invalid after the outer function returns.

It is most of the times preferable to explicitly spell out all variables in a lambda capture, because it forces you to think about if you want to capture by reference or value.

> +			if (map.is_resource_valid(egbase.world(), coords, resIx)) {
> +				return coords.field->nodecaps();
> +			}
> +			return 0;
> +		});
>  
>  	map.recalc_whole_map(egbase.world());
>  	select_correct_tool();


-- 
https://code.launchpad.net/~widelands-dev/widelands/fix_overlays/+merge/281641
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fix_overlays.