widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #17644
Re: [Merge] lp:~widelands-dev/widelands/unify-program-parsers into lp:widelands
Looking mostly good, two comments in the diff.
I went through it commit by commit while ignoring the merges of trunk and list-directories-in-cpp. I hope that is okay? I am a bit unsure about that since there seem to be changes in the full diff that I can't find in the single commits.
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;
Is silently doing nothing the right behavior here? Shouldn't it throw a wrong_gamestate or something like that?
> + }
> +
> + // 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--;
You dropped the line with
it->second--;
while refactoring. Was this on purpose? It seems to be the line that really does something in this loop.
Due to this change(?), it seems like half-build ships are no longer rotting away with this branch.
> - 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.
References
