[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1729856 into lp:widelands

2017-11-06 Thread noreply 
The proposal to merge lp:~widelands-dev/widelands/bug-1729856 into lp:widelands 
has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1729856.

___
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-1729856 into lp:widelands

2017-11-06 Thread GunChleoc 
I think that's a good solution, it gives a bit more detail than the assert. So, 
we can still get a crash with regressions in debug builds, while in release 
builds people can keep playing, because it's not that bad a bug.

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1729856.

___
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-1729856 into lp:widelands

2017-11-05 Thread TiborB 
Well, neither is good solution - throw gives details, but affects also 
"production" release, so I opted for combined solution: log+assert, what do you 
think?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1729856.

___
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-1729856 into lp:widelands

2017-11-05 Thread GunChleoc 
The proposal to merge lp:~widelands-dev/widelands/bug-1729856 into lp:widelands 
has been updated.

Commit Message changed to:

Added a check to the AI when considering the upgrade of trainingsites to make 
sure we are not going to exceed the limit for weaker AIs.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1729856.

___
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-1729856 into lp:widelands

2017-11-05 Thread GunChleoc 
Review: Approve

LGTM, 1 nit. Do you want to change the exception back to an assert?

Diff comments:

> === modified file 'src/ai/defaultai_warfare.cc'
> --- src/ai/defaultai_warfare.cc   2017-11-02 19:55:17 +
> +++ src/ai/defaultai_warfare.cc   2017-11-04 20:46:09 +
> @@ -576,14 +576,24 @@
>   TrainingSiteObserver& tso = trainingsites.front();
>  
>   // Make sure we are not above ai type limit
> - assert(tso.bo->total_count() <= tso.bo->cnt_limit_by_aimode);
> + if (tso.bo->total_count() > tso.bo->cnt_limit_by_aimode) {
> + throw wexception("%d AI count of %s exceeds an AI limit %d: 
> actual count: %d\n",
> +  player_number(), tso.bo->name, 
> tso.bo->cnt_limit_by_aimode,
> +  tso.bo->total_count());
> + }
>  
>   const DescriptionIndex enhancement = ts->descr().enhancement();
>  
>   if (enhancement != INVALID_INDEX && ts_without_trainers_ == 0 && 
> mines_.size() > 3 &&
>   ts_finished_count_ > 1 && ts_in_const_count_ == 0) {
>  
> - if (player_->is_building_type_allowed(enhancement)) {
> + // Make sure that"

// Make sure that" -> // Make sure that

> + // 1. Building is allowed
> + // 2. AI limit for weaker AI is not to be exceeded
> + BuildingObserver& en_bo =
> +
> get_building_observer(tribe_->get_building_descr(enhancement)->name().c_str());
> + if (player_->is_building_type_allowed(enhancement) &&
> + en_bo.aimode_limit_status() == 
> AiModeBuildings::kAnotherAllowed) {
>   game().send_player_enhance_building(*tso.site, 
> enhancement);
>   }
>   }


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1729856.

___
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-1729856 into lp:widelands

2017-11-04 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 2760. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/297351434.
Appveyor build 2572. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1729856-2572.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1729856 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-1729856 into lp:widelands

2017-11-04 Thread TiborB 
TiborB has proposed merging lp:~widelands-dev/widelands/bug-1729856 into 
lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1729856 in widelands: "AI enhances past its building limit"
  https://bugs.launchpad.net/widelands/+bug/1729856

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230

I inserted a check when considering the upgrade of trainingsites to make sure 
we are not going to exceed the limit for weaker AI
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1729856 into lp:widelands.
=== modified file 'src/ai/defaultai_warfare.cc'
--- src/ai/defaultai_warfare.cc	2017-11-02 19:55:17 +
+++ src/ai/defaultai_warfare.cc	2017-11-04 20:46:09 +
@@ -576,14 +576,24 @@
 	TrainingSiteObserver& tso = trainingsites.front();
 
 	// Make sure we are not above ai type limit
-	assert(tso.bo->total_count() <= tso.bo->cnt_limit_by_aimode);
+	if (tso.bo->total_count() > tso.bo->cnt_limit_by_aimode) {
+		throw wexception("%d AI count of %s exceeds an AI limit %d: actual count: %d\n",
+		 player_number(), tso.bo->name, tso.bo->cnt_limit_by_aimode,
+		 tso.bo->total_count());
+	}
 
 	const DescriptionIndex enhancement = ts->descr().enhancement();
 
 	if (enhancement != INVALID_INDEX && ts_without_trainers_ == 0 && mines_.size() > 3 &&
 	ts_finished_count_ > 1 && ts_in_const_count_ == 0) {
 
-		if (player_->is_building_type_allowed(enhancement)) {
+		// Make sure that"
+		// 1. Building is allowed
+		// 2. AI limit for weaker AI is not to be exceeded
+		BuildingObserver& en_bo =
+		   get_building_observer(tribe_->get_building_descr(enhancement)->name().c_str());
+		if (player_->is_building_type_allowed(enhancement) &&
+		en_bo.aimode_limit_status() == AiModeBuildings::kAnotherAllowed) {
 			game().send_player_enhance_building(*tso.site, enhancement);
 		}
 	}

___
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