The proposal to merge lp:~widelands-dev/widelands/request_supply_opt into
lp:widelands has been updated.
Status: Needs review => Merged
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
--
Your team Widelands Developers is subscribed
Review: Approve
there are still NOCOM in the diff which need removing, otherwise lgtm.
Diff comments:
> === modified file 'src/economy/economy.cc'
> --- src/economy/economy.cc2015-11-11 09:52:55 +
> +++ src/economy/economy.cc2016-01-06 20:05:29 +
> @@ -664,14 +665,42 @@
> R
reworked, tested (not long time). Can you please look at it once more?
--
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/request_supply_opt.
__
Changing a reference to a pointer can be a bit of a pain, but it isn't complex
- the compiler will tell you if anything goes wrong, so you can't mess it up.
What you need to do is change foo.bar to foo->bar within the function, and add
& or * in front of foo when calling the function - I can nev
You are right, I will rework it, though I am not that good programmer so it is
not as simple for me as for you. This is only reason why I opted to mimic the
existing code.
But as there is no need to hurry with this branch so it can wait a bit again
--
https://code.launchpad.net/~widelands-dev/w
Tibor, sure, go ahead and merge.
> I looked at other examples f.e. here:
> http://bazaar.launchpad.net/~widelands-dev/widelands/request_supply_opt/view/head:/src/economy/supply.h#L94
> and everywhere it is done this way.
Ah, that is a philosophical question, really. Does local style trump glob
@SirVer
I incorporated your comments, with one exception, can I merge it?
--
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/request_supply_opt.
__
Added a comment to your comment, see in diff
Diff comments:
>
> === modified file 'src/economy/idleworkersupply.cc'
> --- src/economy/idleworkersupply.cc 2015-11-11 09:52:55 +
> +++ src/economy/idleworkersupply.cc 2016-01-04 18:44:46 +
> @@ -71,6 +71,11 @@
> return true;
> }
>
Review: Approve
code lgtm now. A couple of minor nits, feel free to merge after addressing
these.
Diff comments:
> === modified file 'src/economy/economy.cc'
> --- src/economy/economy.cc2015-11-11 09:52:55 +
> +++ src/economy/economy.cc2016-01-04 18:44:46 +
> @@ -664,14 +665,45
> The threshold will at most cut the time in half for find_best_supply if i
> understood correctly.
I dont think there is mathematical reason for 'half' but if it was half it
would be great
It depends on number of supplies - if supplies count<=treshold - no change at
all. Or rather - savin
> So I made a bit of profiling, first of all it seems the game (drawing itself)
> takes quite a lot of CPU time - perhaps problem on my side?
No, that is accurate. The render_queue branch makes this already better, but
only a full texture_atlas will make this really fast. I picked up working on
So I made a bit of profiling, first of all it seems the game (drawing itself)
takes quite a lot of CPU time - perhaps problem on my side? Tests run for 3
minutes of gameplay, but loading took also quite a bit of time (not counted in
3 minutes)
But results:
I compared
1. trunk + crater (smal
[threshold]
I did not follow the discussion about the threshold though, on a quick thought
I can see downsides to this approach. I suggest doing more timing analysis
(with AI disabled) to figure out what actually takes time here and if there are
ways of improving the situation.
> I dont say tha
@SirVer
I reworked it and testing it. I am not satisfied with performance though.
Perhaps you know that I proposed a treshold (f.e. 5) that would check only 5
nearest (as crow fly) wares and pick a one from them. This could improve
performance, but was rejected and currently is not in the code.
Oh, I just read about comparison of tuples, so I understand your example now,
thanks..
--
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/request_supply_opt.
_
Well I am not that skillful in C++, I want(ed) to use trivial:
bool operator() (const uint32_t & p1, const uint32_t & p2) const {
return p1.distance == p2.distance ? p1.serial < p2.serial :
p1.distance < p2.distance;
}
(not tested of course)
--
https://code.launchpad.net/~widelands-de
That should work - but I would suggest to make a tiny private struct to improve
readability.
struct SupplyQuality {
uint32 distance;
uint32 serial;
bool operator<(const SupplyQuality& other) const {
return std::forward_as_tuple(distance, serial) <
std::forward_as_tuple(other.d
@SirVer
What about:
std::multimap<, Supply*> available_supplies;
Where is , serial should be the same
across the network clients, I presume...
--
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
Your team Widelands Developers is subscribed to branch
lp:~wi
Review: Needs Fixing
I think this is broken in the current state. The problem is that logic depends
on available_supply which is a multimap. The problem is that the map is sorted
by distance, then by a pointer.
In a replay or in a multiplayer games, the order of elements in this map is
differ
Review: Approve
LGTM.
I cannot really quantify the performance gain, but i did not find any
regression.
--
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/request_supply_opt.
Resubmit.
I removed controversial treshold, and added two features
a) if more wares (of the same type) on one flag, only one ware is considered
b) wares on ships are ignored. Routing is deceived because it considers port's
flag as a actual location of ware/worker.
--
https://code.launchpad.net/
Forum thread: https://wl.widelands.org/forum/topic/1856/
We still need to think about the threshold, but the rest of the code LGTM :)
This still needs testing, but I think we can wait with that until we have
decided how to solve this.
--
https://code.launchpad.net/~widelands-dev/widelands/reque
TiborB has proposed merging lp:~widelands-dev/widelands/request_supply_opt into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/request_supply_opt/+merge/280193
This is small redesign of delivery
23 matches
Mail list logo
