Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/00_private_inheritance into lp:widelands

2017-07-11 Thread GunChleoc


Diff comments:

> 
> === modified file 'src/logic/cmd_luacoroutine.cc'
> --- src/logic/cmd_luacoroutine.cc 2017-01-25 18:55:59 +
> +++ src/logic/cmd_luacoroutine.cc 2017-07-05 21:50:35 +
> @@ -51,11 +53,11 @@
>   log("%s\n", e.what());
>   log("Send message to all players and pause game\n");
>   for (int i = 1; i <= game.map().get_nrplayers(); i++) {
> - Widelands::Message& msg = *new Widelands::Message(
> + std::unique_ptrmsg(new Widelands::Message(

Thanks, no need to fix this stuff manually then :)

>  Message::Type::kGameLogic, game.get_gametime(), 
> "Coroutine",
>  "images/ui_basic/menu_help.png", "Lua Coroutine 
> Failed",
> -(boost::format("%s") % 
> e.what()).str());
> - game.player(i).add_message(game, msg, true);
> +(boost::format("%s") % 
> e.what()).str()));
> + game.player(i).add_message(game, std::move(msg), true);
>   }
>   game.game_controller()->set_desired_speed(0);
>   }


-- 
https://code.launchpad.net/~widelands-dev/widelands/00_private_inheritance/+merge/326886
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/00_private_inheritance.

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

2017-07-11 Thread GunChleoc
A couple of nits, code LGTM otherwise. Needs testing.

Diff comments:

> 
> === modified file 'data/campaigns/bar01.wmf/scripting/mission_thread.lua'
> --- data/campaigns/bar01.wmf/scripting/mission_thread.lua 2016-12-29 
> 10:31:07 +
> +++ data/campaigns/bar01.wmf/scripting/mission_thread.lua 2017-07-09 
> 10:09:27 +
> @@ -15,15 +15,21 @@
>  -- =
>  
>  function introduction_thread()
> +   reveal_concentric(plr, sf, 13)
> sleep(2000)
>  
> message_box_objective(plr, briefing_msg_01)
> -- these buildings are still burning, but only for a while
> map:place_immovable("destroyed_building",map:get_field(7,41),"tribes")
> map:place_immovable("destroyed_building",map:get_field(5,52),"tribes")
> -   plr:reveal_fields(al_thunran:region(8))
> +   -- plr:reveal_fields(al_thunran:region(8))

Remove commented out line.

> +   scroll_to_field(al_thunran)
> +   reveal_concentric(plr, al_thunran, 8, true, 50)
> message_box_objective(plr, briefing_msg_02) -- Al'thunran
> -   plr:reveal_fields(grave:region(4))
> +   scroll_to_field(sf)
> +   sleep(1000)
> +   scroll_to_field(grave)
> +   reveal_concentric(plr, grave, 4)
> message_box_objective(plr, briefing_msg_03) -- grave, Boldreth
> message_box_objective(plr, briefing_msg_04) -- wait
> -- introduction of Khantrukh
> 
> === modified file 'data/campaigns/bar02.wmf/scripting/mission_thread.lua'
> --- data/campaigns/bar02.wmf/scripting/mission_thread.lua 2017-01-17 
> 08:52:14 +
> +++ data/campaigns/bar02.wmf/scripting/mission_thread.lua 2017-07-09 
> 10:09:27 +
> @@ -11,6 +12,9 @@
>  fr2 = game.map:get_field(85,1)
>  fr3 = game.map:get_field(85,11)
>  
> +-- Starting field
> +sf = game.map.player_slots[1].starting_field

Check if this also works with local sf = ... local is always better.

> +
>  function check_conquered_footprints()
>  if p1:seen_field(game.map:get_field(65, 28))
>  then
> 
> === modified file 'data/campaigns/emp02.wmf/scripting/mission_thread.lua'
> --- data/campaigns/emp02.wmf/scripting/mission_thread.lua 2017-03-16 
> 21:54:07 +
> +++ data/campaigns/emp02.wmf/scripting/mission_thread.lua 2017-07-09 
> 10:09:27 +
> @@ -111,16 +113,16 @@
> -- Reveal the other mountains
> local coal_mountain = wl.Game().map:get_field(49,22)
> local iron_mountain = wl.Game().map:get_field(38,37)
> -   p1:reveal_fields(coal_mountain:region(6))
> +
> +   --local move_point = wl.Game().map:get_field(49,22)

Remove commented out line.

> +   wait_for_roadbuilding_and_scroll(coal_mountain)
> +   reveal_concentric(p1, coal_mountain, 6, false)
> p1:reveal_fields(iron_mountain:region(6))
> run(function() sleep(5000)
>p1:hide_fields(coal_mountain:region(6))
>p1:hide_fields(iron_mountain:region(6))
> end)
>  
> -   local move_point = wl.Game().map:get_field(49,22)
> -   wait_for_roadbuilding_and_scroll(move_point)
> -
> campaign_message_box(saledus_3)
> p1:allow_buildings{
>"empire_coalmine",
> 
> === added file 'data/scripting/field_animations.lua'
> --- data/scripting/field_animations.lua   1970-01-01 00:00:00 +
> +++ data/scripting/field_animations.lua   2017-07-09 10:09:27 +
> @@ -0,0 +1,173 @@
> +-- RST
> +-- .. _field_animations:
> +--
> +-- field_animations.lua
> +-- 
> +--
> +-- This script contain some animations to reveal and hide fields seen 

contain -> contains

> +-- by a player. This functions are currently used in the campaigns and 
> scenarios
> +-- to tell the prologue to a story.
> +
> +-- RST
> +-- .. function:: reveal_randomly(player, region, time)
> +--
> +--Reveal a given region field by field, where the fields 
> +--are chosen randomly. The region get hidden prior revealing.

"The region get hidden prior revealing." - I do not understand this sentence. 
The region will be hidden before being revealed?

> +--The animation runs the specified time.
> +--See also :meth:`wl.map.Field.region`
> +--
> +--:arg player: The player who get sight to the region

get -> gets

> +--:arg region: The region that has to be revealed
> +--:type region: :class:`array` of :class:`wl.map.Fields`
> +--:arg time: Optional. The time the whole animation will run. 
> +--   Defaults to 1000 (1 sec)
> +
> +function reveal_randomly(plr, region, time)
> +   -- If no 'time' is given use a default
> +   time = time or 1000
> +
> +   -- Make sure the region is hidden
> +   plr:hide_fields(region, true)
> +
> +   -- Turn off buildhelp during animation
> +   local buildhelp_state = wl.ui.MapView().buildhelp
> +   if buildhelp_state then
> +  wl.ui.MapView().buildhelp = false
> +   end
> +   
> +   -- Calculate delay as integer
> +   local delay = math.floor(time / #region)
> +   -- Make sure 'delay' is valid
> +   if delay < 1 then delay = 1 end
> +
> +   -- Reveal field by field
> +   while #region > 0 do
> +  l

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reveal_hide_animations into lp:widelands

2017-07-11 Thread kaputtnik
Many thanks for the review :-)

Regarding the use of 'local' i added local to the other variables too and 
tested this.

Of course i have tested all the changes, but having an additional test is 
better.
-- 
https://code.launchpad.net/~widelands-dev/widelands/reveal_hide_animations/+merge/327062
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/reveal_hide_animations 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