Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/workarea-fixes into lp:widelands
Review: Approve LGTM :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/371171 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
inputqueues @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
Review: Approve Retested and still working :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
Sorry for the comment spam tested and working nicely. I am also happy with the colors and borders :) -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
2 comments for code readability
Diff comments:
>
> === modified file 'src/logic/map_objects/tribes/tribes.cc'
> --- src/logic/map_objects/tribes/tribes.cc2019-05-29 06:24:42 +
> +++ src/logic/map_objects/tribes/tribes.cc2019-06-05 14:12:33 +
> @@ -335,6 +335,21 @@
> for (const auto& job : de->working_positions()) {
>
> workers_->get_mutable(job.first)->add_employer(i);
> }
> +
> + for (const auto& pair :
> de->get_highlight_overlapping_workarea_for()) {
Please add a comment above this loop to describe what it's for.
> + const DescriptionIndex di =
> safe_building_index(pair.first);
> + if (upcast(const ProductionSiteDescr, p,
> get_building_descr(di))) {
> + if (!p->workarea_info().empty()) {
> + continue;
> + }
> + throw GameDataError(
> + "Productionsite %s will
> inform about conflicting building %s which doesn’t have a workarea",
> + de->name().c_str(),
> pair.first.c_str());
> + }
> + throw GameDataError(
> + "Productionsite %s will inform
> about conflicting building %s which is not a productionsite",
> + de->name().c_str(),
> pair.first.c_str());
> + }
> }
>
> // Register which buildings buildings can have been enhanced
> from
>
> === modified file 'src/wui/fieldaction.cc'
> --- src/wui/fieldaction.cc2019-05-31 19:31:45 +
> +++ src/wui/fieldaction.cc2019-06-05 14:12:33 +
> @@ -768,11 +779,14 @@
> continue;
> }
> const Widelands::BuildingDescr* d =
> nullptr;
> + bool positive;
Turn this into bool& again and then pass bool* to the functions. This will make
the code easier to read.
> if (imm_type ==
> Widelands::MapObjectType::CONSTRUCTIONSITE) {
>
> upcast(Widelands::ConstructionSite, cs, imm);
> d = cs->get_info().becomes;
> if ((descr.type() ==
> Widelands::MapObjectType::PRODUCTIONSITE &&
> - d->type() !=
> Widelands::MapObjectType::PRODUCTIONSITE) ||
> + (d->type() !=
> Widelands::MapObjectType::PRODUCTIONSITE ||
> + !dynamic_cast Widelands::ProductionSiteDescr&>(descr).
> +
> highlight_overlapping_workarea_for(d->name(), positive))) ||
> ((descr.type() ==
> Widelands::MapObjectType::MILITARYSITE ||
> descr.type() ==
> Widelands::MapObjectType::WAREHOUSE) &&
>imm_type !=
> Widelands::MapObjectType::MILITARYSITE &&
--
https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
Don't worry, kaputtnik, we can't always agree on everything and your opinion is just as valid as everybody else's -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
ok, ok :-) Sorry for the noise. Will be quiet now. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
OK, Lua entries it is. And I am in favor of this feature too - it will come in handy, especially for less-experienced players. I think we could have an explanation in the basic control tutorial, after the quarries have been placed and before messages get introduced. Don't do this in trunk or in this branch though, because I have a merge request open that introduces big changes to the tutorial code, and the merge conflicts would be eek. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
It was definitely wanted by more then one player. I like it too. Especially if it shows good or bad coverage. I think the basic tutorial should bhe the place to explain this as early as possible, cause it will be already there. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
Good idea, will implement that :) > On the other side: Is it worth the work for a feature > - which is hard to explain I can just add an explanation in a tutorial. Would the economy tut or bar01 be the better place for it? > - which was wanted by a single player I would not have implemented it so soon if I didn´t want it as well… ;) > - were other players know how to handle the 'problem' without this feature …by clicking a dozen times and still overlooking something. I like this feature better than that ;) -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
Definitions look good from my side so far. one additional question / demand? Would it be possible to distinguish between wanted overlaps (e.g. forester and lumberjack shown in greenish colour) and unwanted overlaps (e.g. farm and forester shown in red shadows) -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
> Hm on second thought - the beekeeper -> fields rule might be actually covered > by rule 4 The farm plants only barley_field_tiny, but the flowering attrib belongs to barley_field_medium. We would have to check not only whether they plant the same attrib, but also whether one plants something that turns into something that turns into something … that has the attrib the other collects. Since it´s only a few entries per building, that would be much more effort than doing it in Lua… -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
Hm on second thought - the beekeeper -> fields rule might be actually covered by rule 4, so that would be an argument in favor of the rule-based approach, because we already found a bug in this branch ;) -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
While i am not convinced by this feature at all, i agree with Gun. The lua files are already full of tables and options, which make it hard to understand for beginners (modders). On the other side: Is it worth the work for a feature - which is hard to explain - which was wanted by a single player - were other players know how to handle the 'problem' without this feature -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
As pointed out by hessenfarmer in https://bugs.launchpad.net/widelands/+bug/1830345/comments/17, The beekeeper would be interested in farms and berries too. So, let's keep the Lua design and add those to the building. A rule-based approach would get very complicated with that. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
I'm still in favor of deducing these in the engine so that we won't need to maintain these Lua definitions, which will be prone to bugs. Criteria could be: 1. If both buildings mine the same resource 2. If both buildings plant an immovable 3. If both buildings collect an immovable 4. If one building plants an immovable and the other collects it and it is the same attrib 5. If both buildings plant or collect the same bob attrib If we want to go this route, we will need to base it off https://code.launchpad.net/~widelands-dev/widelands/unify-program-parsers/+merge/367936 in order not to get merge conflicts. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368342 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
This time it´s just the inputqueues @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
Review: Approve :P Looks good though :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/workarea-fixes. ___ 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/workarea-fixes into lp:widelands
Done. The diff is now almost 50% longer :P -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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/workarea-fixes into lp:widelands
Code LGTM and it's all working well :) Could you make the surrounding line thicker? It's too thin to be noticeable, really. -- https://code.launchpad.net/~widelands-dev/widelands/workarea-fixes/+merge/368083 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/workarea-fixes 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
