widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Thanks for the review! It should be OK. There were some merge conflicts, which is why there might be some small additional stuff, but nothing serious.

I have pushed a commit to address your comments.

Diff comments:

> 
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc	2019-04-26 05:52:49 +0000
> +++ src/logic/editor_game_base.cc	2019-06-06 06:00:01 +0000
> @@ -113,50 +113,57 @@
>   * throws an exception if something goes wrong
>   */
>  void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const type) {
> -	// should only be called when a map was already loaded
> -	assert(map_.filesystem());
> -
> -	g_fs->ensure_directory_exists(kTempFileDir);
> -
> -	std::string filename = kTempFileDir + g_fs->file_separator() + timestring() + "_mapdata";
> -	std::string complete_filename = filename + kTempFileExtension;
> -
> -	// if a file with that name already exists, then try a few name modifications
> -	if (g_fs->file_exists(complete_filename)) {
> -		int suffix;
> -		for (suffix = 0; suffix <= 9; suffix++) {
> -			complete_filename = filename + "-" + std::to_string(suffix) + kTempFileExtension;
> -			if (!g_fs->file_exists(complete_filename))
> -				break;
> -		}
> -		if (suffix > 9) {
> -			throw wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all considered "
> -			                 "filenames a file already existed");
> -		}
> -	}
> -
> -	// create tmp_fs_
> -	tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> -
> -	// save necessary map data (we actually save the whole map)
> -	std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*tmp_fs_, *this));
> -	wms->save();
> -
> -	// swap map fs
> -	std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
> -	map_.swap_filesystem(mapfs);
> -	mapfs.reset();
> -
> -	// This is just a convenience hack:
> -	// If tmp_fs_ is a zip filesystem then - because of the way zip filesystems are currently
> -	// implemented -
> -	// the file is still in zip mode right now, which means that the file isn't finalized yet, i.e.,
> -	// not even a valid zip file until zip mode ends. To force ending the zip mode (thus finalizing
> -	// the file)
> -	// we simply perform a (otherwise useless) filesystem request.
> -	// It's not strictly necessary, but this way we get a valid zip file immediately istead of
> -	// at some unkown later point (when an unzip operation happens or a filesystem object destructs).
> -	tmp_fs_->file_exists("binary");
> +	if (!map_.filesystem()) {
> +		return;

It's the same behaviour as in trunk, but you're right - it's not even needed. Removed the whole if clause.

> +	}
> +
> +	// save map data to temporary file and reassign map fs
> +	try {
> +		g_fs->ensure_directory_exists(kTempFileDir);
> +
> +		std::string filename = kTempFileDir + g_fs->file_separator() + timestring() + "_mapdata";
> +		std::string complete_filename = filename + kTempFileExtension;
> +
> +		// if a file with that name already exists, then try a few name modifications
> +		if (g_fs->file_exists(complete_filename)) {
> +			int suffix;
> +			for (suffix = 0; suffix <= 9; suffix++) {
> +				complete_filename = filename + "-" + std::to_string(suffix) + kTempFileExtension;
> +				if (!g_fs->file_exists(complete_filename))
> +					break;
> +			}
> +			if (suffix > 9) {
> +				throw wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all considered "
> +								 "filenames a file already existed");
> +			}
> +		}
> +
> +		// create tmp_fs_
> +		tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> +
> +		// save necessary map data (we actually save the whole map)
> +		std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*tmp_fs_, *this));
> +		wms->save();
> +
> +		// swap map fs
> +		std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
> +		map_.swap_filesystem(mapfs);
> +		mapfs.reset();
> +
> +		// This is just a convenience hack:
> +		// If tmp_fs_ is a zip filesystem then - because of the way zip filesystems are currently
> +		// implemented -
> +		// the file is still in zip mode right now, which means that the file isn't finalized yet, i.e.,
> +		// not even a valid zip file until zip mode ends. To force ending the zip mode (thus finalizing
> +		// the file)
> +		// we simply perform a (otherwise useless) filesystem request.
> +		// It's not strictly necessary, but this way we get a valid zip file immediately istead of
> +		// at some unkown later point (when an unzip operation happens or a filesystem object destructs).
> +		tmp_fs_->file_exists("binary");
> +	} catch (const WException& e) {
> +		log("EditorGameBase: saving map to temporary file failed: %s", e.what());
> +		throw;
> +	}
>  }
>  
>  void EditorGameBase::think() {
> 
> === modified file 'src/logic/map_objects/immovable.cc'
> --- src/logic/map_objects/immovable.cc	2019-05-26 17:21:15 +0000
> +++ src/logic/map_objects/immovable.cc	2019-06-06 06:00:01 +0000
> @@ -761,391 +697,6 @@
>  	return loader.release();
>  }
>  
> -ImmovableProgram::Action::~Action() {
> -}
> -
> -ImmovableProgram::ActAnimate::ActAnimate(char* parameters, ImmovableDescr& descr) {
> -	try {
> -		bool reached_end;
> -		animation_name_ = std::string(next_word(parameters, reached_end));
> -		if (!descr.is_animation_known(animation_name_)) {
> -			throw GameDataError("Unknown animation: %s.", animation_name_.c_str());
> -		}
> -
> -		if (!reached_end) {  //  The next parameter is the duration.
> -			char* endp;
> -			long int const value = strtol(parameters, &endp, 0);
> -			if (*endp || value <= 0)
> -				throw GameDataError("expected %s but found \"%s\"", "duration in ms", parameters);
> -			duration_ = value;
> -		} else {
> -			duration_ = 0;  //  forever
> -		}
> -	} catch (const WException& e) {
> -		throw GameDataError("animate: %s", e.what());
> -	}
> -}
> -
> -/// Use convolutuion to make the animation time a random variable with binomial
> -/// distribution and the configured time as the expected value.
> -void ImmovableProgram::ActAnimate::execute(Game& game, Immovable& immovable) const {
> -	immovable.start_animation(game, immovable.descr().get_animation(animation_name_, &immovable));
> -	immovable.program_step(
> -	   game, duration_ ? 1 + game.logic_rand() % duration_ + game.logic_rand() % duration_ : 0);
> -}
> -
> -ImmovableProgram::ActPlaySound::ActPlaySound(char* parameters, const ImmovableDescr&) {
> -	try {
> -		bool reached_end;
> -		std::string name = next_word(parameters, reached_end);
> -
> -		if (!reached_end) {
> -			char* endp;
> -			unsigned long long int const value = strtoull(parameters, &endp, 0);
> -			priority = value;
> -			if (*endp || priority != value)
> -				throw GameDataError("expected %s but found \"%s\"", "priority", parameters);
> -		} else
> -			priority = 127;
> -
> -		fx = g_sh->register_fx(SoundType::kAmbient, name);
> -	} catch (const WException& e) {
> -		throw GameDataError("playsound: %s", e.what());
> -	}
> -}
> -
> -/** Demand from the g_sound_handler to play a certain sound effect.
> - * Whether the effect actually gets played
> - * is decided only by the sound server*/
> -void ImmovableProgram::ActPlaySound::execute(Game& game, Immovable& immovable) const {
> -	Notifications::publish(NoteSound(SoundType::kAmbient, fx, immovable.get_position(), priority));
> -	immovable.program_step(game);
> -}
> -
> -ImmovableProgram::ActTransform::ActTransform(char* parameters, ImmovableDescr& descr) {
> -	try {
> -		tribe = true;
> -		bob = false;
> -		probability = 0;
> -
> -		std::vector<std::string> params = split_string(parameters, " ");
> -		for (uint32_t i = 0; i < params.size(); ++i) {
> -			if (params[i] == "bob")
> -				bob = true;
> -			else if (params[i] == "immovable")
> -				bob = false;
> -			else if (params[i][0] >= '0' && params[i][0] <= '9') {
> -				long int const value = atoi(params[i].c_str());
> -				if (value < 1 || 254 < value)
> -					throw GameDataError("expected %s but found \"%s\"", "probability in range [1, 254]",
> -					                    params[i].c_str());
> -				probability = value;
> -			} else {
> -				std::vector<std::string> segments = split_string(params[i], ":");
> -
> -				if (segments.size() > 2)
> -					throw GameDataError("object type has more than 2 segments");
> -				if (segments.size() == 2) {
> -					if (segments[0] == "world")
> -						tribe = false;
> -					else if (segments[0] == "tribe") {
> -						if (descr.owner_type() != MapObjectDescr::OwnerType::kTribe)
> -							throw GameDataError("scope \"tribe\" does not match the immovable type");
> -						tribe = true;
> -					} else
> -						throw GameDataError("unknown scope \"%s\" given for target type (must be "
> -						                    "\"world\" or \"tribe\")",
> -						                    parameters);
> -
> -					type_name = segments[1];
> -				} else {
> -					type_name = segments[0];
> -				}
> -			}
> -		}
> -		if (type_name == descr.name())
> -			throw GameDataError("illegal transformation to the same type");
> -	} catch (const WException& e) {
> -		throw GameDataError("transform: %s", e.what());
> -	}
> -}
> -
> -void ImmovableProgram::ActTransform::execute(Game& game, Immovable& immovable) const {
> -	if (probability == 0 || game.logic_rand() % 256 < probability) {
> -		Player* player = immovable.get_owner();
> -		Coords const c = immovable.get_position();
> -		MapObjectDescr::OwnerType owner_type = immovable.descr().owner_type();
> -		immovable.remove(game);  //  Now immovable is a dangling reference!
> -
> -		if (bob) {
> -			game.create_ship(c, type_name, player);
> -		} else {
> -			game.create_immovable_with_name(
> -			   c, type_name, owner_type, player, nullptr /* former_building_descr */);
> -		}
> -	} else
> -		immovable.program_step(game);
> -}
> -
> -ImmovableProgram::ActGrow::ActGrow(char* parameters, ImmovableDescr& descr) {
> -	if (!descr.has_terrain_affinity()) {
> -		throw GameDataError(
> -		   "Immovable %s can 'grow', but has no terrain_affinity entry.", descr.name().c_str());
> -	}
> -
> -	try {
> -		tribe = true;
> -		for (char* p = parameters;;)
> -			switch (*p) {
> -			case ':': {
> -				*p = '\0';
> -				++p;
> -				if (descr.owner_type() != MapObjectDescr::OwnerType::kTribe)
> -					throw GameDataError("immovable type not in tribes but target type has scope "
> -					                    "(\"%s\")",
> -					                    parameters);
> -				else if (strcmp(parameters, "world"))
> -					throw GameDataError("scope \"%s\" given for target type (must be "
> -					                    "\"world\")",
> -					                    parameters);
> -				tribe = false;
> -				parameters = p;
> -				break;
> -			}
> -			case '\0':
> -				goto end;
> -			default:
> -				++p;
> -				break;
> -			}
> -	end:
> -		type_name = parameters;
> -	} catch (const WException& e) {
> -		throw GameDataError("grow: %s", e.what());
> -	}
> -}
> -
> -void ImmovableProgram::ActGrow::execute(Game& game, Immovable& immovable) const {
> -	const Map& map = game.map();
> -	FCoords const f = map.get_fcoords(immovable.get_position());
> -	const ImmovableDescr& descr = immovable.descr();
> -
> -	if ((game.logic_rand() % TerrainAffinity::kPrecisionFactor) <
> -	    probability_to_grow(descr.terrain_affinity(), f, map, game.world().terrains())) {
> -		MapObjectDescr::OwnerType owner_type = descr.owner_type();
> -		Player* owner = immovable.get_owner();
> -		immovable.remove(game);  //  Now immovable is a dangling reference!
> -		game.create_immovable_with_name(
> -		   f, type_name, owner_type, owner, nullptr /* former_building_descr */);
> -	} else {
> -		immovable.program_step(game);
> -	}
> -}
> -
> -/**
> - * remove
> - */
> -ImmovableProgram::ActRemove::ActRemove(char* parameters, ImmovableDescr&) {
> -	try {
> -		if (*parameters) {
> -			char* endp;
> -			long int const value = strtol(parameters, &endp, 0);
> -			if (*endp || value < 1 || 254 < value)
> -				throw GameDataError(
> -				   "expected %s but found \"%s\"", "probability in range [1, 254]", parameters);
> -			probability = value;
> -		} else
> -			probability = 0;
> -	} catch (const WException& e) {
> -		throw GameDataError("remove: %s", e.what());
> -	}
> -}
> -
> -void ImmovableProgram::ActRemove::execute(Game& game, Immovable& immovable) const {
> -	if (probability == 0 || game.logic_rand() % 256 < probability)
> -		immovable.remove(game);  //  Now immovable is a dangling reference!
> -	else
> -		immovable.program_step(game);
> -}
> -
> -ImmovableProgram::ActSeed::ActSeed(char* parameters, ImmovableDescr& descr) {
> -	try {
> -		probability = 0;
> -		for (char* p = parameters;;)
> -			switch (*p) {
> -			case ':': {
> -				*p = '\0';
> -				++p;
> -				if (descr.owner_type() != MapObjectDescr::OwnerType::kTribe)
> -					throw GameDataError("immovable type not in tribes but target type has scope "
> -					                    "(\"%s\")",
> -					                    parameters);
> -				else if (strcmp(parameters, "world"))
> -					throw GameDataError("scope \"%s\" given for target type (must be "
> -					                    "\"world\")",
> -					                    parameters);
> -				parameters = p;
> -				break;
> -			}
> -			case ' ': {
> -				*p = '\0';
> -				++p;
> -				char* endp;
> -				long int const value = strtol(p, &endp, 0);
> -				if (*endp || value < 1 || 254 < value)
> -					throw GameDataError(
> -					   "expected %s but found \"%s\"", "probability in range [1, 254]", p);
> -				probability = value;
> -				//  fallthrough
> -			}
> -				FALLS_THROUGH;
> -			case '\0':
> -				goto end;
> -			default:
> -				++p;
> -				break;
> -			}
> -	end:
> -		type_name = parameters;
> -	} catch (const WException& e) {
> -		throw GameDataError("seed: %s", e.what());
> -	}
> -}
> -
> -void ImmovableProgram::ActSeed::execute(Game& game, Immovable& immovable) const {
> -	const Map& map = game.map();
> -	FCoords const f = map.get_fcoords(immovable.get_position());
> -	const ImmovableDescr& descr = immovable.descr();
> -
> -	if ((game.logic_rand() % TerrainAffinity::kPrecisionFactor) <
> -	    probability_to_grow(descr.terrain_affinity(), f, map, game.world().terrains())) {
> -		// Seed a new tree.
> -		MapFringeRegion<> mr(map, Area<>(f, 0));
> -		uint32_t fringe_size = 0;
> -		do {
> -			mr.extend(map);
> -			fringe_size += 6;
> -		} while (game.logic_rand() % std::numeric_limits<uint8_t>::max() < probability);
> -
> -		for (uint32_t n = game.logic_rand() % fringe_size; n; --n) {
> -			mr.advance(map);
> -		}
> -
> -		const FCoords new_location = map.get_fcoords(mr.location());
> -		if (!new_location.field->get_immovable() &&
> -		    (new_location.field->nodecaps() & MOVECAPS_WALK) &&
> -		    (game.logic_rand() % TerrainAffinity::kPrecisionFactor) <
> -		       probability_to_grow(
> -		          descr.terrain_affinity(), new_location, map, game.world().terrains())) {
> -			game.create_immovable_with_name(mr.location(), type_name, descr.owner_type(),
> -			                                nullptr /* owner */, nullptr /* former_building_descr */);
> -		}
> -	}
> -
> -	immovable.program_step(game);
> -}
> -
> -ImmovableProgram::ActConstruct::ActConstruct(char* parameters, ImmovableDescr& descr) {
> -	try {
> -		if (descr.owner_type() != MapObjectDescr::OwnerType::kTribe)
> -			throw GameDataError("only usable for tribe immovable");
> -
> -		std::vector<std::string> params = split_string(parameters, " ");
> -
> -		if (params.size() != 3)
> -			throw GameDataError("usage: animation-name buildtime decaytime");
> -
> -		buildtime_ = atoi(params[1].c_str());
> -		decaytime_ = atoi(params[2].c_str());
> -
> -		animation_name_ = params[0];
> -		if (!descr.is_animation_known(animation_name_)) {
> -			throw GameDataError("unknown animation \"%s\" in immovable program for immovable \"%s\"",
> -			                    animation_name_.c_str(), descr.name().c_str());
> -		}
> -	} catch (const WException& e) {
> -		throw GameDataError("construct: %s", e.what());
> -	}
> -}
> -
> -constexpr uint8_t kCurrentPacketVersionConstructionData = 1;
> -
> -struct ActConstructData : ImmovableActionData {
> -	const char* name() const override {
> -		return "construct";
> -	}
> -	void save(FileWrite& fw, Immovable& imm) override {
> -		fw.unsigned_8(kCurrentPacketVersionConstructionData);
> -		delivered.save(fw, imm.get_owner()->tribe());
> -	}
> -
> -	static ActConstructData* load(FileRead& fr, Immovable& imm) {
> -		ActConstructData* d = new ActConstructData;
> -
> -		try {
> -			uint8_t packet_version = fr.unsigned_8();
> -			if (packet_version == kCurrentPacketVersionConstructionData) {
> -				d->delivered.load(fr, imm.get_owner()->tribe());
> -			} else {
> -				throw UnhandledVersionError(
> -				   "ActConstructData", packet_version, kCurrentPacketVersionConstructionData);
> -			}
> -		} catch (const WException& e) {
> -			delete d;
> -			d = nullptr;
> -			throw GameDataError("ActConstructData: %s", e.what());
> -		}
> -
> -		return d;
> -	}
> -
> -	Buildcost delivered;
> -};
> -
> -void ImmovableProgram::ActConstruct::execute(Game& g, Immovable& imm) const {
> -	ActConstructData* d = imm.get_action_data<ActConstructData>();
> -	if (!d) {
> -		// First execution
> -		d = new ActConstructData;
> -		imm.set_action_data(d);
> -
> -		imm.start_animation(g, imm.descr().get_animation(animation_name_, &imm));
> -		imm.anim_construction_total_ = imm.descr().buildcost().total();
> -	} else {
> -		// Perhaps we are called due to the construction timeout of the last construction step
> -		Buildcost remaining;
> -		imm.construct_remaining_buildcost(g, &remaining);
> -		if (remaining.empty()) {
> -			imm.program_step(g);
> -			return;
> -		}
> -
> -		// Otherwise, this is a decay timeout
> -		uint32_t totaldelivered = 0;
> -		for (Buildcost::const_iterator it = d->delivered.begin(); it != d->delivered.end(); ++it)
> -			totaldelivered += it->second;
> -
> -		if (!totaldelivered) {
> -			imm.remove(g);
> -			return;
> -		}
> -
> -		uint32_t randdecay = g.logic_rand() % totaldelivered;
> -		for (Buildcost::iterator it = d->delivered.begin(); it != d->delivered.end(); ++it) {
> -			if (randdecay < it->second) {
> -				it->second--;

It was on purpose, but it was wrong. I misread it->second-- for it--. I have put this back in.

> -				break;
> -			}
> -
> -			randdecay -= it->second;
> -		}
> -
> -		imm.anim_construction_done_ = d->delivered.total();
> -	}
> -
> -	imm.program_step_ = imm.schedule_act(g, decaytime_);
> -}
> -
>  /**
>   * For an immovable that is currently in construction mode, return \c true and
>   * compute the remaining buildcost.


-- 
https://code.launchpad.net/~widelands-dev/widelands/unify-program-parsers/+merge/367936
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/unify-program-parsers into lp:widelands.