Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/oars_appdata into lp:widelands
Review: Needs Fixing One more things: The specification you mentioned is for the *.desktop files not *.appdata.xml files. The appdata specification does not mention the type property so I think we can trust the validator and remove it. -- https://code.launchpad.net/~widelands-dev/widelands/oars_appdata/+merge/309624 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/oars_appdata. ___ 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/oars_appdata into lp:widelands
Review: Needs Fixing LBTM. This is invalid XML, you have multiple root tags. The has to be a child of . You can use the appstream package of your distribution to validate appstream files. E.g: appstreamcli validate debian/widelands.appdata.xml It throws a lot of errors, even before your change but you can still use it to see if new validation errors where introduced. -- https://code.launchpad.net/~widelands-dev/widelands/oars_appdata/+merge/309624 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/oars_appdata. ___ 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-1624216-horsepocalypse into lp:widelands
It could be coded a bit more elegantly(see my comments), but apart from that it
LGTM. However, I did not test it because I dont know a straight forward way to
get a warehous with 500+ workers.
Diff comments:
> === modified file 'src/logic/map_objects/tribes/warehouse.cc'
> --- src/logic/map_objects/tribes/warehouse.cc 2016-08-04 15:49:05 +
> +++ src/logic/map_objects/tribes/warehouse.cc 2016-11-07 19:19:08 +
> @@ -528,13 +529,24 @@
> if (upcast(Game, game, &egbase)) {
> const WareList& workers = get_workers();
> for (DescriptionIndex id = 0; id < workers.get_nrwareids();
> ++id) {
> - const uint32_t stock = workers.stock(id);
> + Quantity stock = workers.stock(id);
> // Separate behaviour for the case of loading the game
> // (which does save/destroy/reload) and simply
> destroying ingame
> if (game->is_loaded()) {
> // This game is really running
> - for (uint32_t i = 0; i < stock; ++i) {
> - launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + Quantity worker_counter = 0;
I dont see why we need "worker_counter" and "i". I suggest to initialize
"worker_counter" within for() and get rid of "i"
> + for (Quantity i = 0; i < stock; ++i,
> ++worker_counter) {
> + // Make sure that we won't flood the
> map with carriers etc.
> + if (worker_counter < kFleeingUnitsCap) {
This condition - which is part of the exit condition - should be in the for
statement and not in the loop. E.g. sth like
for (Quantity worker_counter = 0; worker_counter < stock && worker_counter <
kFleeingUnitsCap; ++worker_counter).
If implemented like this we dont need the break statement.
> + launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + } else {
> + break;
> + }
> + }
> + // Remove the remaining stock in case we hit
> the cap
> + stock = workers.stock(id);
> + if (stock > 0) {
> + remove_workers(id, stock);
> }
> assert(!incorporated_workers_.count(id) ||
> incorporated_workers_[id].empty());
> } else {
If we had a method remove_all_workers we wouldnt need to differntiate between
load-game and in-game. We could just remove all workers after the
if(game->is_loaded()) condition. So IMHO the cleanest solution would be to
implement this method, but I understand if you dont want to do this. Its not
nessecary to fix this bug.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1624216-horsepocalypse/+merge/310221
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1624216-horsepocalypse 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands
Janosch Peters has proposed merging
lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1638394 in widelands: "Crash on rendering roads"
https://bugs.launchpad.net/widelands/+bug/1638394
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1638394-render-road/+merge/310715
If the owner of the starting field of the road is not visible, the owner of the
end field of the road is used to determine the road texture in
RoadProgram::add_road.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands.
=== modified file 'src/graphic/gl/road_program.cc'
--- src/graphic/gl/road_program.cc 2016-10-24 20:07:22 +
+++ src/graphic/gl/road_program.cc 2016-11-13 08:26:42 +
@@ -72,12 +72,17 @@
const float road_thickness_x = (-delta_y / vector_length) * kRoadThicknessInPixels * scale;
const float road_thickness_y = (delta_x / vector_length) * kRoadThicknessInPixels * scale;
- assert(start.owner != nullptr);
+ assert(start.owner != nullptr || end.owner != nullptr);
+
+ Widelands::Player* visible_owner = start.owner;
+ if (start.owner == nullptr) {
+ visible_owner = end.owner;
+ }
+
const Image& texture =
- road_type == Widelands::RoadType::kNormal ?
- start.owner->tribe().road_textures().get_normal_texture(
- start.geometric_coords, direction) :
- start.owner->tribe().road_textures().get_busy_texture(start.geometric_coords, direction);
+ road_type == Widelands::RoadType::kNormal ?
+ visible_owner->tribe().road_textures().get_normal_texture(start.geometric_coords, direction) :
+ visible_owner->tribe().road_textures().get_busy_texture(start.geometric_coords, direction);
if (*gl_texture == 0) {
*gl_texture = texture.blit_data().texture_id;
}
___
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:~janosch-peters/widelands/bug-1638890-segfault-harbour into lp:widelands
The proposal to merge lp:~janosch-peters/widelands/bug-1638890-segfault-harbour into lp:widelands has been updated. Description changed to: The harbour now conqueres every location where its military influence is higher than the influence of other players. This fixes the bug if there is a portdock location available where the player owning the harbour has highest influence. IMHO this will almost always be the case. If all portdock locations are occupied by another players influence, then it is very likely that the player owning the harbour could not have build the harbour in the first place. For more details, see: https://code.launchpad.net/~janosch-peters/widelands/bug-1638890-segfault-harbour/+merge/310092 -- Your team Widelands Developers is requested to review the proposed merge of lp:~janosch-peters/widelands/bug-1638890-segfault-harbour 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
[Widelands-dev] [Merge] lp:~janosch-peters/widelands/bug-1638890-segfault-harbour into lp:widelands
Janosch Peters has proposed merging
lp:~janosch-peters/widelands/bug-1638890-segfault-harbour into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1638890 in widelands: "Segfault right after colony harbour is
constructed"
https://bugs.launchpad.net/widelands/+bug/1638890
For more details, see:
https://code.launchpad.net/~janosch-peters/widelands/bug-1638890-segfault-harbour/+merge/310092
The harbour now conqueres eevry location where its military influence is higher
than the influence of other players.
This fixes the if there is a portdock location available where the player
owning the harbour has highest influence. IMHO this will almost always be the
case. If all portdock locations are occupied by another players influence, then
it is very likely that the player owning the harbour could not have build the
harbour in the first place.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~janosch-peters/widelands/bug-1638890-segfault-harbour into lp:widelands.
=== modified file 'src/logic/editor_game_base.cc'
--- src/logic/editor_game_base.cc 2016-10-21 08:21:41 +
+++ src/logic/editor_game_base.cc 2016-11-04 18:09:15 +
@@ -529,7 +529,7 @@
/// This conquers a given area because of a new (military) building that is set
/// there.
-void EditorGameBase::conquer_area(PlayerArea> player_area) {
+void EditorGameBase::conquer_area(PlayerArea> player_area, bool conquer_guarded_location) {
assert(0 <= player_area.x);
assert(player_area.x < map().get_width());
assert(0 <= player_area.y);
@@ -539,7 +539,7 @@
assert(0 < player_area.player_number);
assert(player_area.player_number <= map().get_nrplayers());
- do_conquer_area(player_area, true);
+ do_conquer_area(player_area, true, 0, conquer_guarded_location);
// Players are not allowed to have their immovables on their borders.
// Therefore the area must be enlarged before calling
@@ -603,10 +603,10 @@
// testsuite).
void EditorGameBase::do_conquer_area(PlayerArea> player_area,
bool const conquer,
- PlayerNumber const preferred_player,
+ PlayerNumber const preferred_player,
+ bool const conquer_guarded_location_by_superior_influence,
bool const neutral_when_no_influence,
- bool const neutral_when_competing_influence,
- bool const conquer_guarded_location_by_superior_influence) {
+ bool const neutral_when_competing_influence) {
assert(0 <= player_area.x);
assert(player_area.x < map().get_width());
assert(0 <= player_area.y);
=== modified file 'src/logic/editor_game_base.h'
--- src/logic/editor_game_base.h 2016-09-30 09:56:42 +
+++ src/logic/editor_game_base.h 2016-11-04 18:09:15 +
@@ -171,7 +171,7 @@
void inform_players_about_road(FCoords, MapObjectDescr const*);
void unconquer_area(PlayerArea>, PlayerNumber destroying_player = 0);
- void conquer_area(PlayerArea>);
+ void conquer_area(PlayerArea>, bool conquer_guarded_location = false);
void conquer_area_no_building(PlayerArea> const);
void cleanup_objects() {
@@ -238,10 +238,10 @@
/// influence becomes greater than the owner's influence.
virtual void do_conquer_area(PlayerArea> player_area,
bool conquer,
- PlayerNumber preferred_player = 0,
+ PlayerNumber preferred_player = 0,
+ bool conquer_guarded_location_by_superior_influence = false,
bool neutral_when_no_influence = false,
- bool neutral_when_competing_influence = false,
- bool conquer_guarded_location_by_superior_influence = false);
+ bool neutral_when_competing_influence = false);
void cleanup_playerimmovables_area(PlayerArea>);
// Changes the owner of 'fc' from the current player to the new player and
=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc 2016-08-04 15:49:05 +
+++ src/logic/map_objects/tribes/warehouse.cc 2016-11-04 18:09:15 +
@@ -425,7 +425,7 @@
if (uint32_t const conquer_radius = descr().get_conquers()) {
egbase.conquer_area(PlayerArea>(
player.player_number(),
- Area(egbase.map().get_fcoords(get_position()), conquer_radius)));
+ Area(egbase.map().get_fcoords(get_position()), conquer_radius)), true);
}
if (descr().get_isport()) {
___
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/fix_overlays into lp:widelands
Review: Approve Tho code look good, sorry for introducing this bug. About the resource-editing thing: Strictly speaking I think the behaviour is consistent. The analogous feature of "remove immovable" in terms of resources is "set resource to 0". This works even if you have a different resource selected. The decrease/increase resource has no equivalent. After a little bit of testing I also think the current increase/decrease behaviour makes sense. Imaging you have several resource sitting next to another. If you have a bigger tool size and start to reduce resources, it should only affect the resource you have selected. If we change the behaviour to allow decreasing any resource, at least in my scenario this will lead to unexpected results. -- https://code.launchpad.net/~widelands-dev/widelands/fix_overlays/+merge/281641 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fix_overlays. ___ 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
