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. _______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp