widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I have done a first round of code review, no testing yet.

Diff comments:

> 
> === modified file 'src/editor/tools/delete_critter_tool.cc'
> --- src/editor/tools/delete_critter_tool.cc	2019-02-23 11:00:49 +0000
> +++ src/editor/tools/delete_critter_tool.cc	2019-08-23 14:48:43 +0000
> @@ -41,7 +40,7 @@
>  	do
>  		if (Widelands::Bob* const bob = mr.location().field->get_first_bob()) {
>  			args->old_bob_type.push_back(&bob->descr());
> -			bob->remove(egbase);
> +			bob->remove(eia.egbase());

We have egbase as a function parameter, so no need to get it from eia

>  		} else {
>  			args->old_bob_type.push_back(nullptr);
>  		}
> 
> === modified file 'src/editor/tools/delete_immovable_tool.cc'
> --- src/editor/tools/delete_immovable_tool.cc	2019-02-23 11:00:49 +0000
> +++ src/editor/tools/delete_immovable_tool.cc	2019-08-23 14:48:43 +0000
> @@ -28,18 +28,17 @@
>  /**
>   * Deletes the immovable at the given location
>   */
> -int32_t EditorDeleteImmovableTool::handle_click_impl(const Widelands::World&,
> +int32_t EditorDeleteImmovableTool::handle_click_impl(const Widelands::EditorGameBase&,
>                                                       const Widelands::NodeAndTriangle<>& center,
> -                                                     EditorInteractive& parent,
> +                                                     EditorInteractive& eia,
>                                                       EditorActionArgs* args,
>                                                       Widelands::Map* map) {
> -	Widelands::EditorGameBase& egbase = parent.egbase();
>  	Widelands::MapRegion<Widelands::Area<Widelands::FCoords>> mr(
>  	   *map, Widelands::Area<Widelands::FCoords>(map->get_fcoords(center.node), args->sel_radius));
>  	do
>  		if (upcast(Widelands::Immovable, immovable, mr.location().field->get_immovable())) {
>  			args->old_immovable_types.push_back(immovable->descr().name());
> -			immovable->remove(egbase);  //  Delete no buildings or stuff.
> +			immovable->remove(eia.egbase());  //  Delete no buildings or stuff.

We have egbase as a function parameter, so no need to get it from eia

>  		} else {
>  			args->old_immovable_types.push_back("");
>  		}
> 
> === modified file 'src/editor/tools/resize_tool.cc'
> --- src/editor/tools/resize_tool.cc	2019-04-24 07:09:29 +0000
> +++ src/editor/tools/resize_tool.cc	2019-08-23 14:48:43 +0000
> @@ -43,18 +42,17 @@
>  	args->resized.deleted_fields =
>  	   map->resize(egbase, center.node, args->new_map_size.w, args->new_map_size.h);
>  
> -	map->recalc_whole_map(world);
> +	map->recalc_whole_map(egbase);
>  	return 0;
>  }
>  
>  int32_t
> -EditorResizeTool::handle_undo_impl(const Widelands::World& world,
> +EditorResizeTool::handle_undo_impl(const Widelands::EditorGameBase&,
>                                     const Widelands::NodeAndTriangle<Widelands::Coords>& center,
> -                                   EditorInteractive& parent,
> +                                   EditorInteractive& eia,
>                                     EditorActionArgs* args,
>                                     Widelands::Map* map) {
> -	Widelands::EditorGameBase& egbase = parent.egbase();
> -
> +	Widelands::EditorGameBase& egbase = eia.egbase();

Delete this line - we are already passing in EditorGameBase as a function argument; use that. Please check all editor tools to see that the code is cleaned up for the new function parameter.

>  	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) {
> 
> === modified file 'src/logic/map.cc'
> --- src/logic/map.cc	2019-06-25 08:03:30 +0000
> +++ src/logic/map.cc	2019-08-23 14:48:43 +0000
> @@ -168,18 +168,19 @@
>  			check_neighbour_heights(f, radius);
>  			recalc_brightness(f);
>  			recalc_border(f);
> -			recalc_nodecaps_pass1(world, f);
> +			recalc_nodecaps_pass1(egbase, f);
>  		}
>  
>  	for (int16_t y = 0; y < height_; ++y)
>  		for (int16_t x = 0; x < width_; ++x) {
>  			f = get_fcoords(Coords(x, y));
> -			recalc_nodecaps_pass2(world, f);
> +			recalc_nodecaps_pass2(egbase, f);
>  		}
>  	recalculate_allows_seafaring();
>  }
>  
> -void Map::recalc_default_resources(const World& world) {
> +void Map::recalc_default_resources(const EditorGameBase& egbase) {

We only need the world in this function, so this change can be reverted.

> +	const World& world = egbase.world();
>  	for (int16_t y = 0; y < height_; ++y)
>  		for (int16_t x = 0; x < width_; ++x) {
>  			FCoords f, f1;
> @@ -2121,16 +2124,17 @@
>  
>  	// Changing the terrain can affect ports, which can be up to 3 fields away.
>  	constexpr int kPotentiallyAffectedNeighbors = 3;
> -	recalc_for_field_area(world, Area<FCoords>(c.node, kPotentiallyAffectedNeighbors));
> +	recalc_for_field_area(egbase, Area<FCoords>(c.node, kPotentiallyAffectedNeighbors));
>  	return kPotentiallyAffectedNeighbors;
>  }
>  
> -bool Map::is_resource_valid(const Widelands::World& world,
> +bool Map::is_resource_valid(const Widelands::EditorGameBase& egbase,

This change can be reverted, only the World is being used.

>                              const Widelands::FCoords& c,
>                              DescriptionIndex curres) const {
>  	if (curres == Widelands::kNoResource)
>  		return true;
>  
> +	const World& world = egbase.world();
>  	Widelands::FCoords f1;
>  
>  	int32_t count = 0;
> 
> === modified file 'src/logic/map_objects/findnode.cc'
> --- src/logic/map_objects/findnode.cc	2019-02-27 19:00:36 +0000
> +++ src/logic/map_objects/findnode.cc	2019-08-23 14:48:43 +0000
> @@ -55,11 +58,12 @@
>  	return true;
>  }
>  
> -bool FindNodeSize::accept(const Map&, const FCoords& coord) const {
> +bool FindNodeSize::accept(const EditorGameBase& egbase, const FCoords& coord) const {

If we had the map and the world here as parameters, we would not need huge changes to the codebase to add the egbase everywhere.

Also, this is how it works for other special nodes:

FindNodeAnd functor;
	functor.add(FindNodeSize(static_cast<FindNodeSize::Size>(action.iparam2)));
	if (action.sparam1.size()) {
		if (action.iparam4)
			functor.add(FindNodeResourceBreedable(world.get_resource(action.sparam1.c_str())));
		else
			functor.add(FindNodeResource(world.get_resource(action.sparam1.c_str())));
	}

I think we should go along with that design and replace the boolean check with an enum class, then we can revert all changes to FindNodeCaps::accept.

>  	if (BaseImmovable const* const immovable = coord.field->get_immovable())
>  		if (immovable->get_size() > BaseImmovable::NONE)
>  			return false;
>  	NodeCaps const nodecaps = coord.field->nodecaps();
> +	const Map& map = egbase.map();
>  
>  	switch (size) {
>  	case sizeBuild:
> @@ -74,13 +78,22 @@
>  		return (nodecaps & BUILDCAPS_SIZEMASK) >= BUILDCAPS_MEDIUM;
>  	case sizeBig:
>  		return (nodecaps & BUILDCAPS_SIZEMASK) >= BUILDCAPS_BIG;
> +	case sizeSwim:
> +		return map.can_reach_by_water(coord);
> +	case sizeTerraform:

Have a new functor FindNodeResourceTerraformable instead(see above).

> +		return !(egbase.world().terrain_descr(coord.field->terrain_d()).enhancement().empty() &&
> +				egbase.world().terrain_descr(coord.field->terrain_r()).enhancement().empty() &&
> +				egbase.world().terrain_descr(map.tl_n(coord).field->terrain_d()).enhancement().empty() &&
> +				egbase.world().terrain_descr(map.tl_n(coord).field->terrain_r()).enhancement().empty() &&
> +				egbase.world().terrain_descr(map.tr_n(coord).field->terrain_d()).enhancement().empty() &&
> +				egbase.world().terrain_descr(map.l_n(coord).field->terrain_r()).enhancement().empty());
>  	case sizeAny:
>  		return true;
>  	}
>  	NEVER_HERE();
>  }
>  
> -bool FindNodeImmovableSize::accept(const Map&, const FCoords& coord) const {
> +bool FindNodeImmovableSize::accept(const EditorGameBase&, const FCoords& coord) const {
>  	int32_t size = BaseImmovable::NONE;
>  
>  	if (BaseImmovable* const imm = coord.field->get_immovable())
> 
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc	2019-06-25 08:03:30 +0000
> +++ src/logic/map_objects/tribes/building.cc	2019-08-23 14:48:43 +0000
> @@ -107,6 +108,12 @@
>  			throw GameDataError("size: %s", e.what());
>  		}
>  	}
> +	if (table.has_key("built_over_immovable")) {
> +		// Throws an error if no matching immovable exists
> +		built_over_immovable_ = get_attribute_id(table.get_string("built_over_immovable"), false);
> +	} else {
> +		built_over_immovable_ = -1;

Please use Widelands::INVALID_INDEX

> +	}
>  
>  	// Parse build options
>  	if (table.has_key("enhancement")) {
> @@ -172,12 +179,48 @@
>                                  Coords const pos,
>                                  bool const construct,
>                                  bool loading,
> -                                Building::FormerBuildings const former_buildings) const {
> +                                FormerBuildings const former_buildings) const {
> +	std::pair<DescriptionIndex, std::string> immovable = std::make_pair(INVALID_INDEX, "");
> +	if (built_over_immovable_ >= 0) {
> +		for (const auto& pair : former_buildings) {
> +			if (!pair.second.empty()) {
> +				const MapObjectDescr* d;
> +				if (pair.second == "world") {
> +					d = egbase.world().get_immovable_descr(pair.first);
> +				} else if (pair.second == "tribe") {
> +					d = egbase.tribes().get_immovable_descr(pair.first);
> +				} else {
> +					throw wexception("Invalid FormerBuildings type: %s", pair.second.c_str());
> +				}
> +				if (d->has_attribute(built_over_immovable_)) {
> +					goto immovable_previously_found;

Please try to avoid gotos https://homepages.cwi.nl/~storm/teaching/reader/Dijkstra68.pdf

> +				}
> +			}
> +		}
> +		// Must be done first, because the immovable will be gone the moment the building is placed
> +		const FCoords f = egbase.map().get_fcoords(pos);
> +		if (f.field->get_immovable() && f.field->get_immovable()->has_attribute(built_over_immovable_)) {
> +			upcast(const ImmovableDescr, imm, &f.field->get_immovable()->descr());
> +			assert(imm);
> +			immovable = imm->owner_type() == MapObjectDescr::OwnerType::kWorld ?
> +					std::make_pair(egbase.world().get_immovable_index(imm->name()), "world") :
> +					std::make_pair(egbase.tribes().safe_immovable_index(imm->name()), "tribe");
> +		} else {
> +			throw wexception("Attempting to build %s at %dx%d – no immovable with required attribute %i found",
> +					name().c_str(), pos.x, pos.y, built_over_immovable_);
> +		}
> +	}
> +	immovable_previously_found:
> +
>  	Building& b = construct ? create_constructionsite() : create_object();
>  	b.position_ = pos;
>  	b.set_owner(owner);
> -	for (DescriptionIndex idx : former_buildings) {
> -		b.old_buildings_.push_back(idx);
> +	if (immovable.first != INVALID_INDEX) {
> +		assert(!immovable.second.empty());
> +		b.old_buildings_.push_back(immovable);
> +	}
> +	for (const auto& pair : former_buildings) {
> +		b.old_buildings_.push_back(pair);
>  	}
>  	if (loading) {
>  		b.Building::init(egbase);
> @@ -188,8 +231,10 @@
>  }
>  
>  bool BuildingDescr::suitability(const Map&, const FCoords& fc) const {
> -	return mine_ ? fc.field->nodecaps() & Widelands::BUILDCAPS_MINE :
> -	               size_ <= (fc.field->nodecaps() & Widelands::BUILDCAPS_SIZEMASK);
> +	return (mine_ ? fc.field->nodecaps() & Widelands::BUILDCAPS_MINE :
> +	               size_ <= (fc.field->nodecaps() & Widelands::BUILDCAPS_SIZEMASK)) &&
> +           (built_over_immovable_ < 0 ||

Use INVALID_INDEX

> +                   (fc.field->get_immovable() && fc.field->get_immovable()->has_attribute(built_over_immovable_)));
>  }
>  
>  const BuildingHints& BuildingDescr::hints() const {
> 
> === modified file 'src/logic/map_objects/tribes/constructionsite.cc'
> --- src/logic/map_objects/tribes/constructionsite.cc	2019-06-23 11:41:17 +0000
> +++ src/logic/map_objects/tribes/constructionsite.cc	2019-08-23 14:48:43 +0000
> @@ -196,16 +197,23 @@
>  	   NoteSound(SoundType::kAmbient, descr().creation_fx(), position_, kFxPriorityAlwaysPlay));
>  	PartiallyFinishedBuilding::init(egbase);
>  
> -	const std::map<DescriptionIndex, uint8_t>* buildcost;
> +	const std::map<DescriptionIndex, uint8_t>* buildcost = nullptr;
>  	if (!old_buildings_.empty()) {
> -		// Enhancement
> -		DescriptionIndex was_index = old_buildings_.back();
> -		const BuildingDescr* was_descr = owner().tribe().get_building_descr(was_index);
> -		info_.was = was_descr;
> -		buildcost = &building_->enhancement_cost();
> -	} else {
> +		// Enhancement and/or built over immovable
> +		for (auto it = old_buildings_.end(); it != old_buildings_.begin();) {
> +			--it;

This can go into the for statement, just like you'd do with ++it if you were iterating from the front.

> +			if (it->second.empty()) {
> +				const BuildingDescr* was_descr = owner().tribe().get_building_descr(it->first);
> +				info_.was = was_descr;
> +				buildcost = &building_->enhancement_cost();
> +				break;
> +			}
> +		}
> +	}
> +	if (!buildcost) {
>  		buildcost = &building_->buildcost();
>  	}
> +	assert(buildcost);
>  
>  	//  TODO(unknown): figure out whether planing is necessary
>  
> @@ -609,9 +617,13 @@
>                              float scale,
>                              RenderTarget* dst) {
>  	uint32_t tanim = gametime - animstart_;
> -	// Draw the construction site marker
>  	const RGBColor& player_color = get_owner()->get_playercolor();
> -	dst->blit_animation(point_on_dst, Widelands::Coords::null(), scale, anim_, tanim, &player_color);
> +	if (was_immovable_) {
> +		dst->blit_animation(point_on_dst, coords, scale, was_immovable_->main_animation(), tanim, &player_color);
> +	} else {
> +		// Draw the construction site marker

Do we want to blit the constructionsite marker in addition to the tree? We could experiment with that to see how it looks.

> +		dst->blit_animation(point_on_dst, Widelands::Coords::null(), scale, anim_, tanim, &player_color);
> +	}
>  
>  	// Draw the partially finished building
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/amazons-coding-changes/+merge/371725
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/amazons-coding-changes into lp:widelands.