widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'src/logic/map_objects/immovable.cc'
> --- src/logic/map_objects/immovable.cc	2016-11-03 07:20:57 +0000
> +++ src/logic/map_objects/immovable.cc	2016-11-03 07:36:43 +0000
> @@ -339,6 +339,7 @@
>  
>  Immovable::Immovable(const ImmovableDescr& imm_descr)
>     : BaseImmovable(imm_descr),
> +     antecedent_(nullptr),

I am not familiar with this word. Can you find a simpler name?

>       anim_(0),
>       animstart_(0),
>       program_(nullptr),
> 
> === modified file 'src/logic/map_objects/immovable.h'
> --- src/logic/map_objects/immovable.h	2016-11-03 07:20:57 +0000
> +++ src/logic/map_objects/immovable.h	2016-11-03 07:36:43 +0000
> @@ -201,7 +201,11 @@
>  	Immovable(const ImmovableDescr&);
>  	~Immovable();
>  
> -	void set_owner(Player*);
> +	/// If this immovable was created by a building, this can be set in order to display information
> +	/// about it. If this is a player immovable, you will need to set the owner first.
> +	void set_antecedent(const BuildingDescr& building);
> +
> +	void set_owner(Player* player);

I think this function is overwritten in base classes. You'll have a bad time if you do not mark it as virtual here. Ideally you want to remove the overwritten functions too.

>  
>  	Coords get_position() const {
>  		return position_;
> 
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc	2016-11-03 07:20:57 +0000
> +++ src/logic/map_objects/tribes/building.cc	2016-11-03 07:36:43 +0000
> @@ -441,8 +442,12 @@
>  	const Coords pos = position_;
>  	PlayerImmovable::destroy(egbase);
>  	// We are deleted. Only use stack variables beyond this point
> -	if (fire)
> -		egbase.create_immovable(pos, "destroyed_building", MapObjectDescr::OwnerType::kTribe);
> +	if (fire) {
> +		Immovable& destroyed_building =
> +		   egbase.create_immovable(pos, "destroyed_building", MapObjectDescr::OwnerType::kTribe);

You'll probably end up with cleaner code if you get rid of 'set_owner' and 'set_antcedent' and instead add a 'create_immovable_with_precedessor' in EditorGame and set up all the state in the constructor of Immovable and/or in the EgBase function.

> +		destroyed_building.set_owner(get_owner());
> +		destroyed_building.set_antecedent(descr());
> +	}
>  }
>  
>  std::string Building::info_string(const InfoStringFormat& format) {


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.