Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/mines-worldsavior into lp:widelands
Review: Resubmit Ok yesterday I fixed all diff comments from Nordfriese and me. (Hope I caught all of them) Did a testrun of 4 Ai (one of each tribe) on Full moon map. No anomalies showed up in the first 3 hours. Although no mine got depleted so I couldn't get the stats of a depleted mine. Ai was working ok. Did some updates to my excel calculations about the needed Buildings to supply the soldier production. I uploaded the excel file to the frisians balancing bug. The calculations showed that we needed to adjust frisian weapon production times a lot. I entered values of comparable weapons of the other tribes. And added them to lua. Furthermore I discovered that we should set the trainingsite percentage of at least one trainingsite for each tribe due to their different full cycle time (e.g. for barbarians the trainingcamp does need 240 seconds for a whole cycle while the arena does need only 60 which means the arena could supply 4 camps with fully trained soldiers) so I fixed this on the fly. >From my side the branch could be ready now pending further review of my late >changes. -- https://code.launchpad.net/~widelands-dev/widelands/mines-worldsavior/+merge/350716 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/mines-worldsavior. ___ 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/no-hardcoded-resources into lp:widelands
Continuous integration builds have changed state: Travis build 3723. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/408099801. Appveyor build 3523. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_no_hardcoded_resources-3523. -- https://code.launchpad.net/~widelands-dev/widelands/no-hardcoded-resources/+merge/350750 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/no-hardcoded-resources 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/bug-1736901-shift-market into lp:widelands
Review: Approve LGTM :-) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1736901-shift-market/+merge/349630 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1736901-shift-market. ___ 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/no-hardcoded-resources into lp:widelands
Continuous integration builds have changed state: Travis build 3722. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/407977888. Appveyor build 3522. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_no_hardcoded_resources-3522. -- https://code.launchpad.net/~widelands-dev/widelands/no-hardcoded-resources/+merge/350750 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/no-hardcoded-resources 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/bug-1619402-port-work-area-on-expedition into lp:widelands
Review: Needs Fixing review, compile
Going to compile and tesplay this now ...
For the Review I find:
* Renamed: show_work_area -> show_workarea, Same for hide_work_area ->
hide_workarea
* Option show_workarea_preview_ / "Show buildings area preview" removed
* "workareapreview" not checked any longer
* Added new: is_warping_ - no idea what this indicates?
Some questions inline.
There is a bug in src/wui/interactive_player.cc:335:20: error: use of undeclared
identifier 'work_area_overlays'; did you mean 'workarea_overlays'?
const auto it = work_area_overlays.find(f->fcoords);
^~
workarea_overlays
(travis faild as of the bug in trunk we had before)
Shall I try to fix this?
Otherwise code LGTM.
Diff comments:
>
> === modified file 'src/wui/buildingwindow.cc'
> --- src/wui/buildingwindow.cc 2018-04-27 06:11:05 +
> +++ src/wui/buildingwindow.cc 2018-07-22 19:53:33 +
> @@ -58,6 +58,7 @@
> building_position_(b.get_position()),
> showing_workarea_(false),
> avoid_fastclick_(avoid_fastclick),
> + is_warping_(false),
How is this related to the workarea? What will be the effect?
> expeditionbtn_(nullptr) {
> buildingnotes_subscriber_ =
> Notifications::subscribe(
> [this](const Widelands::NoteBuilding& note) {
> on_building_note(note); });
>
> === modified file 'src/wui/buildingwindow.h'
> --- src/wui/buildingwindow.h 2018-04-27 06:11:05 +
> +++ src/wui/buildingwindow.h 2018-07-22 19:53:33 +
> @@ -129,6 +129,7 @@
>
> bool showing_workarea_;
> bool avoid_fastclick_;
> + bool is_warping_;
please add comment like: warping means: objets will move faster than light ;-)
>
> UI::Button* expeditionbtn_;
>
> std::unique_ptr>
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1619402-port-work-area-on-expedition/+merge/349594
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1619402-port-work-area-on-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/no-hardcoded-resources into lp:widelands
I implemented your renaming suggestions. The resource Fish doesn´t need a timeout because it´s used only for geologist messages, not by buildings that mine this resource. timeout is therefore ignored for resources with detectable=false. 1) This is defined in Worker::geologist_update(). The geologist distinguishes between minable and non-minable nodes. If the two triangles attached to his start flag are both minable, he´ll investigate only minable spots, else only non-minable. 2) Resource indicators currently have to be named "resi_". This is defined in TribeDescr::get_resource_indicator(). The indicator´s name has to match exactly this format but no resource names are hardcoded. This should be changed in a follow-up branch to either have the tribe specify a set of immovables for each resource (permitting tribe-specific indicators), or having each resource define a set of (not tribe-specific) indicators. -- https://code.launchpad.net/~widelands-dev/widelands/no-hardcoded-resources/+merge/350750 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/no-hardcoded-resources 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/no-hardcoded-resources into lp:widelands
I like the implementation :) - some nits though.
There are 2 more things that we need to look at:
1. Double-check the worker programs to see how the geologists distinguish
between searching for water vs. mountain resources, to make sure that this is
not hard-coded on the resource type. Maybe this works on terrain type already
anyway, but I don't remember right now.
2. Resource indicators - how do we ensure which resource gets linked to which
resource indicator, and that the tribes have one for each resource? Maybe we
can find a solution here that's more clever, i.e. moving their definitions from
the tribes to the world. An additional check during tribe loading might also be
sufficient here, in case we would like to have the possibility of individual
graphics per tribe in the future.
Point 1 should be looked into for this branch, point 2 can be dealt with
separately.
Diff comments:
> === modified file 'data/world/resources/init.lua'
> --- data/world/resources/init.lua 2017-08-30 17:07:20 +
> +++ data/world/resources/init.lua 2018-07-24 19:34:27 +
> @@ -50,6 +50,18 @@
> --
> --detectable = true,
> --
> +--**timeout_millis**
I'd prefer to have this either spelled out (timeout_milliseconds) or use the
default measuring unit (timeout_ms)
> +--*Mandatory*. Defines the time for which geologists messages for
> this
> +--resource will be muted within this area after a find, e.g.::
> +--
> +--timeout_millis = 3,
> +--
> +--**timeout_radius**
> +--*Mandatory*. Defines the radius within which geologists messages
> for this
> +--resource will be muted after a find, e.g.::
> +--
> +--timeout_radius = 8,
> +--
> --**representative_image**
> --*Mandatory*. Path to an image file that will represent the
> resource in menus
> --etc., e.g.::
> @@ -142,6 +164,8 @@
> descname = _ "Fish",
> max_amount = 20,
> detectable = false,
> + timeout_millis = 0,
We should have a timeout here too, in case the player missed it and it ends up
buried below lots of other messages. All messages will be deleted anyway when
the building is destroyed or dismantled.
> + timeout_radius = 0,
> representative_image = pics_dir .. "fish.png",
> editor_pictures = {
>[5] = pics_dir .. "fish1.png",
>
> === modified file 'src/logic/message.h'
> --- src/logic/message.h 2018-04-07 16:59:00 +
> +++ src/logic/message.h 2018-07-24 19:34:27 +
> @@ -129,15 +131,13 @@
> } else if (type_ >= Widelands::Message::Type::kEconomy &&
> type_ <=
> Widelands::Message::Type::kEconomySiteOccupied) {
> return Widelands::Message::Type::kEconomy;
> - } else if (type_ >= Widelands::Message::Type::kGeologists &&
> -type_ <= Widelands::Message::Type::kGeologistsWater)
> {
> - return Widelands::Message::Type::kGeologists;
> }
> return type_;
> }
>
> private:
> Message::Type type_;
> + const char* type_detail_;
How about calling this "subtype_"?
> const std::string title_;
> const std::string icon_filename_;
> const Image* icon_; // Pointer to icon into picture stack
--
https://code.launchpad.net/~widelands-dev/widelands/no-hardcoded-resources/+merge/350750
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/no-hardcoded-resources 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/fri-01-portspace into lp:widelands
Review: Approve Code LGTM. Should still be tested though, there is always something that could go wrong. -- https://code.launchpad.net/~widelands-dev/widelands/fri-01-portspace/+merge/350729 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fri-01-portspace. ___ 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
