Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191556-cancel-expedition into lp:widelands
Review: Approve Code looks good, still want to play a bit with the Ports on my https://wl.widelands.org/maps/fjord-ilands2/ map. And some refactoring toward clean code is always good :-) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1191556-cancel-expedition/+merge/288375 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1191556-cancel-expedition. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191556-cancel-expedition into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1191556-cancel-expedition into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1191556-cancel-expedition/+merge/288375 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1191556-cancel-expedition. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191556-cancel-expedition into lp:widelands
Thanks for testing :) If you have any ideas concerning refactoring - I am all ears! We could open bugs for those. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1191556-cancel-expedition/+merge/288375 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1191556-cancel-expedition. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1395278-map_io into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1395278-map_io
into lp:widelands.
Commit message:
Refactored member variable names in src/map_io.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1395278 in widelands: "Consolidate naming of member variables"
https://bugs.launchpad.net/widelands/+bug/1395278
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1395278-map_io/+merge/288859
The neverending story...
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1395278-map_io into lp:widelands.
=== modified file 'src/logic/map_revision.cc'
--- src/logic/map_revision.cc 2016-02-22 07:25:15 +
+++ src/logic/map_revision.cc 2016-03-12 16:30:56 +
@@ -25,11 +25,11 @@
MapVersion::MapVersion() :
-map_version_major_(0),
-map_version_minor_(0)
+map_version_major(0),
+map_version_minor(0)
{
- map_creator_version_ = build_id();
- map_version_timestamp_ = static_cast(time(nullptr));
+ map_creator_version = build_id();
+ map_version_timestamp = static_cast(time(nullptr));
}
}
=== modified file 'src/logic/map_revision.h'
--- src/logic/map_revision.h 2016-02-22 07:25:15 +
+++ src/logic/map_revision.h 2016-03-12 16:30:56 +
@@ -34,12 +34,12 @@
struct MapVersion {
- std::string map_source_url_;
- std::string map_source_release_;
- std::string map_creator_version_;
- int32_t map_version_major_;
- int32_t map_version_minor_;
- uint32_tmap_version_timestamp_;
+ std::string map_source_url;
+ std::string map_source_release;
+ std::string map_creator_version;
+ int32_t map_version_major;
+ int32_t map_version_minor;
+ uint32_tmap_version_timestamp;
MapVersion();
=== modified file 'src/map_io/map_elemental_packet.h'
--- src/map_io/map_elemental_packet.h 2014-09-19 12:54:54 +
+++ src/map_io/map_elemental_packet.h 2016-03-12 16:30:56 +
@@ -42,7 +42,7 @@
/// properly configured EditorGameBase object.
void pre_read(FileSystem &, Map *);
- uint32_t get_version() {return m_version;}
+ uint32_t get_version() {return version_;}
/// If this map was created before the one_world merge was done, this returns
/// the old world name, otherwise "".
@@ -52,7 +52,7 @@
private:
std::string old_world_name_;
- uint32_t m_version;
+ uint32_t version_;
};
}
=== modified file 'src/map_io/map_loader.h'
--- src/map_io/map_loader.h 2016-01-29 13:43:56 +
+++ src/map_io/map_loader.h 2016-03-12 16:30:56 +
@@ -43,13 +43,13 @@
};
MapLoader(const std::string& filename, Map & M)
- : m_map(M), m_s(STATE_INIT) {m_map.set_filename(filename);}
+ : map_(M), state_(STATE_INIT) {map_.set_filename(filename);}
virtual ~MapLoader() {}
virtual int32_t preload_map(bool as_scenario) = 0;
virtual int32_t load_map_complete(EditorGameBase &, MapLoader::LoadType) = 0;
- Map & map() {return m_map;}
+ Map & map() {return map_;}
protected:
enum State {
@@ -57,12 +57,12 @@
STATE_PRELOADED,
STATE_LOADED
};
- void set_state(State const s) {m_s = s;}
- State get_state() const {return m_s;}
- Map & m_map;
+ void set_state(State const s) {state_ = s;}
+ State get_state() const {return state_;}
+ Map & map_;
private:
- State m_s;
+ State state_;
};
}
=== modified file 'src/map_io/map_object_loader.cc'
--- src/map_io/map_object_loader.cc 2015-11-29 09:43:15 +
+++ src/map_io/map_object_loader.cc 2016-03-12 16:30:56 +
@@ -29,7 +29,7 @@
* Returns true if this object has already been inserted
*/
bool MapObjectLoader::is_object_known(Serial const n) {
- return m_objects.find(n) != m_objects.end();
+ return objects_.find(n) != objects_.end();
}
@@ -38,7 +38,7 @@
*/
void MapObjectLoader::mark_object_as_loaded(MapObject & obj)
{
- m_loaded_obj[&obj] = true;
+ loaded_objects_[&obj] = true;
}
/*
@@ -48,9 +48,9 @@
{
int32_t result = 0;
std::map::const_iterator const loaded_obj_end =
- m_loaded_obj.end();
+ loaded_objects_.end();
for
- (std::map::const_iterator it = m_loaded_obj.begin();
+ (std::map::const_iterator it = loaded_objects_.begin();
it != loaded_obj_end;
++it)
if (!it->second)
@@ -65,7 +65,7 @@
*/
void MapObjectLoader::schedule_destroy(MapObject & obj)
{
- m_schedule_destroy.push_back(&obj);
+ schedule_destroy_.push_back(&obj);
}
/**
@@ -76,7 +76,7 @@
*/
void MapObjectLoader::schedule_act(Bob & bob)
{
- m_schedule_act.push_back(&bob);
+ schedule_act_.push_back(&bob);
}
/**
@@ -86,14 +86,14 @@
*/
void MapObjectLoader::load_finish_game(Game & g)
{
- while (!m_schedule_destroy.empty()) {
- m_schedule_destroy.back()->schedule_destroy(g);
- m_schedule_destroy.pop_back();
+ while (!schedule_destroy_.empty()) {
+ schedule_destroy_.back()->schedule_destroy(g);
+ schedule_destroy_.pop_back();
}
- while (!m_schedule_act.empty()) {
- m_schedule_act.back()->schedule_act(g, 1);
- m_schedule_act.pop_back();
+ while (!schedule_act_.empty()) {
+ schedule_
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_roads_rework into lp:widelands
OK, I implemented the rest -- https://code.launchpad.net/~widelands-dev/widelands/ai_roads_rework/+merge/288567 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_roads_rework 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_roads_rework into lp:widelands
Review: Approve
LGTM - just 1 more comment in the diff.
Diff comments:
> === modified file 'src/ai/ai_help_structs.cc'
> --- src/ai/ai_help_structs.cc 2016-02-18 17:58:54 +
> +++ src/ai/ai_help_structs.cc 2016-03-12 21:29:28 +
> @@ -300,6 +314,156 @@
> return (blocked_fields_.count(coords.hash()) != 0);
> }
>
> +
> +FlagsForRoads::Candidate::Candidate(uint32_t coords, int32_t distance, bool
> economy):
> + coords_hash(coords), air_distance(distance), different_economy(economy)
> {
> + new_road_possible = false;
> + accessed_via_roads = false;
> + new_road_length = 1000; // Values are sufficient for map 512x512
How about using the map dimensions constant from map.h? If that list would ever
change, whoever changes it will have to know about the AI code. Something like:
new_road_length = 2 * kMapDimensions.at(kMapDimensions.size() - 1)
> + current_roads_distance = 1000; // must be big enough
> + reduction_score = -air_distance; // allows reasonable ordering
> from the start
> +}
> +
> +bool FlagsForRoads::Candidate::operator<(const Candidate& other) const {
> + if (reduction_score == other.reduction_score) {
> + return coords_hash < other.coords_hash;
> + } else {
> + return reduction_score > other.reduction_score;
> + }
> +}
> +
> +bool FlagsForRoads::Candidate::operator==(const Candidate& other) const {
> + return coords_hash == other.coords_hash;
> +}
> +
> +void FlagsForRoads::Candidate::calculate_score() {
> + if (!new_road_possible) {
> + reduction_score = kRoadNotFound - air_distance; // to have at
> least some ordering preserved
> + } else if (different_economy) {
> + reduction_score = kRoadToDifferentEconomy - air_distance - 2 *
> new_road_length;
> + } else if (!accessed_via_roads) {
> + if (air_distance + 6 > new_road_length) {
> + reduction_score = kShortcutWithinSameEconomy -
> air_distance - 2 * new_road_length;
> + } else {
> + reduction_score = kRoadNotFound;
> + }
> + } else {
> + reduction_score = current_roads_distance - 2 * new_road_length;
> + }
> +}
> +
> +void FlagsForRoads::print() { // this is for debugging and development
> purposes
> + for (auto& candidate_flag : queue) {
> + log(" %starget: %3dx%3d, saving: %5d (%3d), air distance:
> %3d, new road: %6d, score: %5d %s\n",
> + (candidate_flag.reduction_score>=min_reduction &&
> candidate_flag.new_road_possible)?"+":" ",
> + Coords::unhash(candidate_flag.coords_hash).x,
> + Coords::unhash(candidate_flag.coords_hash).y,
> + candidate_flag.current_roads_distance -
> candidate_flag.new_road_length,
> + min_reduction,
> + candidate_flag.air_distance,
> + candidate_flag.new_road_length,
> + candidate_flag.reduction_score,
> + (candidate_flag.new_road_possible)? ", new road possible" : "
> ");
> + }
> +}
> +
> +// Queue is ordered but some target flags are only estimations so we take
> such a candidate_flag first
> +bool FlagsForRoads::get_best_uncalculated(uint32_t* winner) {
> + for (auto& candidate_flag : queue) {
> + if (!candidate_flag.new_road_possible) {
> + *winner = candidate_flag.coords_hash;
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +// Road from starting flag to this flag can be built
> +void FlagsForRoads::road_possible(Widelands::Coords coords, uint32_t
> distance) {
> + // std::set does not allow updating
> + Candidate new_candidate_flag = Candidate(0, 0, false);
> + for (auto candidate_flag : queue) {
> + if (candidate_flag.coords_hash == coords.hash()) {
> + new_candidate_flag = candidate_flag;
> + assert(new_candidate_flag.coords_hash ==
> candidate_flag.coords_hash);
> + queue.erase(candidate_flag);
> + break;
> + }
> + }
> +
> + new_candidate_flag.new_road_length = distance;
> + new_candidate_flag.new_road_possible = true;
> + new_candidate_flag.calculate_score();
> + queue.insert(new_candidate_flag);
> +}
> +
> +// Remove the flag from candidates as interconnecting road is not possible
> +void FlagsForRoads::road_impossible(Widelands::Coords coords) {
> + const uint32_t hash = coords.hash();
> + for (auto candidate_flag : queue) {
> + if (candidate_flag.coords_hash == hash) {
> + queue.erase(candidate_flag);
> + return;
> + }
> + }
> +}
> +
> +// Updating walking distance over existing roads
> +// Queue does not allow modifying its members so we erase and then
> eventually insert modified member
> +void FlagsForRoads::set_ro
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_roads_rework into lp:widelands
Or maybe base the value on the actual map dimensions? map.get_width() * map.get_height() -- https://code.launchpad.net/~widelands-dev/widelands/ai_roads_rework/+merge/288567 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ai_roads_rework. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191556-cancel-expedition into lp:widelands
Review: Approve Played this with a lot of Ports and about > 6 Expeditions for some 3 hours. I found a Problem with the network lobby (crash if Computer went offline during the game) but that cannot be related to this code. Compiled it again, but found it is alreday in trunk. I will stick to reviews and testing as long as there are so many branches to check and I do not have that much time. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1191556-cancel-expedition/+merge/288375 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1191556-cancel-expedition. ___ 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
