Merge lp:~widelands-dev/widelands/ship_scheduling_2 into lp:widelands

Proposed by ypopezios
Status: Merged
Merged at revision: 9073
Proposed branch: lp:~widelands-dev/widelands/ship_scheduling_2
Merge into: lp:widelands
Diff against target: 1178 lines (+417/-339)
10 files modified
src/economy/fleet.cc (+196/-219)
src/economy/fleet.h (+6/-1)
src/economy/portdock.cc (+123/-71)
src/economy/portdock.h (+9/-7)
src/economy/shippingitem.cc (+1/-1)
src/economy/shippingitem.h (+1/-1)
src/logic/map_objects/tribes/ship.cc (+71/-32)
src/logic/map_objects/tribes/ship.h (+6/-3)
test/maps/expedition.wmf/scripting/init.lua (+3/-3)
test/maps/expedition.wmf/scripting/test_cancel_when_port_space_was_reached_two_ships.lua (+1/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/ship_scheduling_2
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+355510@code.launchpad.net

This proposal supersedes a proposal from 2018-08-30.

Commit message

Improved ware/worker routing algorithm for ships and portdocks.

Description of the change

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Continuous integration builds have changed state:

Travis build 3754. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/412020649.
Appveyor build 3554. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3554.

Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Continuous integration builds have changed state:

Travis build 3758. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/412187264.
Appveyor build 3558. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3558.

Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Continuous integration builds have changed state:

Travis build 3855. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/422224908.
Appveyor build 3653. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3653.

Revision history for this message
ypopezios (ypopezios) wrote : Posted in a previous version of this proposal

AppVeyor seems broken. Otherwise, this branch is now mature and big enough. Not going to add anything more into it, so as to keep it easier for reviewers. If people test it, it can make it into build 20.

Revision history for this message
hessenfarmer (stephan-lutz) wrote : Posted in a previous version of this proposal

Reason for appveyor not working anymore currently is that the glbinding package has been upgraded in the repo of MSYS from 2.1.4 to 3.0.2.1 which seems to have issues with our code now.

Revision history for this message
GunChleoc (gunchleoc) wrote : Posted in a previous version of this proposal

I have pushed a commit with a code review and some small tweaks. Please have a look at the NOCOM comments.

Not tested yet.

Revision history for this message
ypopezios (ypopezios) wrote : Posted in a previous version of this proposal

Added inline comments concerning the NOCOM comments.

Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Continuous integration builds have changed state:

Travis build 3961. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/428035209.
Appveyor build 3759. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3759.

Revision history for this message
GunChleoc (gunchleoc) wrote : Posted in a previous version of this proposal

I had actually programmed a really nice endless loop with those iterators... all fixed now. I have also improved the savegame compatibility, so it's only calculated once during game loading.

Thanks for this great feature! :)

@bunnybot merge

review: Approve
Revision history for this message
GunChleoc (gunchleoc) wrote : Posted in a previous version of this proposal

One of the tests in our test suite has an endless loop in it:

test/maps/ship_transportation.wmf/scripting/test_rip_ports_with_worker_in_transit.lua

This already happened with older versions of this branch, so I don't think that it's anything I did.

review: Needs Fixing
Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 3977. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/428723984.

Revision history for this message
GunChleoc (gunchleoc) wrote :

There are still failures in the test suite.

Instructions on how to run it manually:

https://wl.widelands.org/wiki/RegressionTests/#test-scenarios

Revision history for this message
ypopezios (ypopezios) wrote :

From what I understand, the test suite forces the removal of ports and ships without running clean-up code. That invalidates the state of transports and breaks the advanced logic. I'm not sure if it is worthy to change the logic, instead of changing the tests.

Revision history for this message
GunChleoc (gunchleoc) wrote :

This removal of ports and ships simulated actions that can be taken by a user, so yes, the new code needs to be able to deal with that. Users can sink ships and destroy ports. Ports can also be destroyed if the enemy conquers the territory.

Revision history for this message
ypopezios (ypopezios) wrote :

Are you sure that there is proper simulation of user-actions here? Cause I suspect that there is just a magical removal, without calling e.g. sinking function. Of course the code should cover cases of destruction, but how to do that if it is not properly called? The old code was checking every time every combination of everything, wasting resources. Do we really want to do things that way?

Revision history for this message
GunChleoc (gunchleoc) wrote :

Yes, the simulation should be correct. Calling the sink_ship function is calling the sink_ship function, and it doesn't matter if the caller is a Lua script or a UI button. Both will send off the same player command. If they don't, it's a bug.

And yes, we do want Widelands not to get into an undefined state when a user performs an action, even if it's sinking a ship or destroying a port. We allow those user actions, so they need to be supported.

I have been thinking about this some more, changes in the test suite should be OK in the following 2 cases:

1. The new algorithm needs a bit more time to get back into a well-defined state than the old algorithm. In this case, add a sleep statement to the test suite.

2. The new algorithm gets back into a well-defined state which is different from the well-defined state that the old algorithm used to have. Change the asserts to reflect the new well-defined state.

In all other cases, the new algorithm needs fixing.

8791. By ypopezios

Address test suite

8792. By ypopezios

Small code-fixes

Revision history for this message
ypopezios (ypopezios) wrote :

This is not perfect, but should be enough for this branch.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4079. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/435223747.
Appveyor build 3875. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3875.

8793. By GunChleoc

Fixed codecheck.

Revision history for this message
ypopezios (ypopezios) wrote :

Concerning the specific TODO, the code for invalid destinations pre-existed my involvement and is suspicious enough. I just moved it earlier in the function, in order to catch more cases. I don't know which are those cases (they don't happen in any part of the code that I touched), only that some of them appear when loading a savegame. I'm not familiar with that part of the codebase, so maybe this TODO is not for me.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4094. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/437518591.
Appveyor build 3890. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ship_scheduling_2-3890.

8794. By GunChleoc

Merged trunk.

Revision history for this message
GunChleoc (gunchleoc) wrote :

The TODOs get tagged with the person who created them, so we can get back to them later in case we don't understand the TODO. It doesn't mean that you have to fix it personally ;)

Test suite is green now, and I have also done a saveloading test with a savegame from trunk.

Running 8 AIs on The Nile on auto_speed slowed down to a crawl before 4 hours gametime, but we are having a problem in trunk (https://bugs.launchpad.net/widelands/+bug/1795976), so it might not be related to this branch.

This will make a great feature for Build 21, thank you! :)

review: Approve
Revision history for this message
ypopezios (ypopezios) wrote :

Thanks for clarifying the TODO tags. I would expect the creator to be written on the left and the assignee on the right of it.

The only way for this branch to slow down the game is if the ports and the ships become hundreds. Even in that case, it should be lighter than the previous code, so I don't think that there is a problem with it.

Did you mean Build 20? I don't see why it has to wait for Build 21. If there is indeed a problem with the code, it would be easier for me to address it now, before the code gets off my brain's cache.

Revision history for this message
GunChleoc (gunchleoc) wrote :

No, I did mean Build 21. We are in feature freeze, and feature freeze means it's feature freeze for everybody. Otherwise, we'll never get a release out, because there will always be that one cool feature we'll want to squeeze in. It's very tempting.

Having an assignee in TODO comment would only make sense if development was paid in this project, which it isn't. Changing the format now will mean changing a bazillion TODO comments in the code base + changing the codecheck rule. I already cleaned them up once and I'm not eager to do it again ;)

Revision history for this message
ypopezios (ypopezios) wrote :

You keep calling that a feature, while it is mostly a fix for the long-standing poor scheduling, so technically it still applies despite the feature freeze. Furthermore, the freeze was announced on 2018-09-17, while this branch has been approved on 2018-09-14. The late problems with the test suite were just that, problems with the test suite, not with the branch. If you insist to be strict with that, you are free to be, but this will inevitably freeze my motivation as well.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Well, I have fully mature branches that have been sitting around for a year that won't make it in because they are still waiting for a code review slot... I could just call them "UI bugs" and insist on having them in Build 20, delaying the release until who knows when, maybe another year?

Every changed line of code has the potential for bugs, and as we have seen from the failing test suite, there are edge cases that don't immediately come up during gameplay testing. You are providing some complex control flow in your branches, so the potential for undiscovered new bugs is high. I have just fixed an endless loop that was introduced in February and only reported last week. So, I don't think that we should risk it with the release coming shortly. Widelands is too mature for the player base to find bugs like that acceptable in a release.

Also, the release procedure that we are using has been developed by my predecessor over the course of > 15 years, so I'm expecting there to be reasons for it to be like it is.

Patience is part of the game, I'm afraid. If you are too demotivated to carry on by having to wait for a few weeks until a feature gets in, here is nothing that we can do about it. I'd be sad to see you leave :'(

Revision history for this message
ypopezios (ypopezios) wrote :

"Widelands is too mature for the player base to find bugs like that acceptable in a release."

Unless you have asked the player-base about the matter, the above statement is arbitrary. If you dare, you can conduct a poll on the issue. Projects much bigger and much more serious than Widelands have no problem releasing code that is not tested in every possible way, exactly to involve the user-base in the testing process (even more so when it comes to complex control flow). The endless loop you fixed after many months, would have been both reported and fixed much earlier if there was a release to expose it to users. Most importantly, the vast majority of modern user-bases has no more problem dealing with minor bugs, as long as they have an easy way to report them and the programmers are eager to fix them in soon-to-follow bug-fixing releases. Users can be as patient as the programmers, especially non-paying users.

Of course all that may leave your "German mentality" untouched, and this is not the place for a theoretical discussion. Still motivation is no less important than patience, and Widelands needs developers at least as much as it needs users. Or so I was told, among other non-truths, like that there are more testers than developers. On the bright side, it is good to know the practical value of my contributions, before undertaking something as big as fixing the routing or the AI.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Please stop insulting everybody whenever you don't get your way. Yes, the release cycles are too long. No, we won't solve the problem of managing testing exposure by merging your feature right now this very instant.

Changing the release process in the middle of a release is not going to happen, because that's far too stressful for me. I'm only human and this is supposed to be a fun hobby. End of discussion.

Revision history for this message
GunChleoc (gunchleoc) wrote :

@bunnybot merge

8795. By GunChleoc

Merged trunk.

8796. By GunChleoc

Merged trunk.

Revision history for this message
ypopezios (ypopezios) wrote :

You can end a discussion, but you cannot help your case. Quite expectedly, the "few weeks" of the "release coming shortly" turned out to be more than half a year (and still counting). Congratulations for the upcoming release, and I wish it to really happen and not to be "too little too late". I also hope that all those months you had your "fun" and maybe you got a lesson on realism. "Widelands is too mature for the player base", but the players base (and even the developers base), although being a beast of patience, it is not mature enough for a project like Widelands. That used to be an educated analysis, but today is a proven reality. Hopefully the persons who got insulted by the analysis, will not get also insulted by reality itself.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Considering all the desyncs and unexpected bugs we were struggling with, do you really think adding a complex new algorithm to the code would have made things any more predictable, or speeded up the release in any way whatsoever? It would have potentially slowed things even further. I'm sorry that's not fast enough for you, but when you work together with other people on a project of this size in our spare time, things don't happen right this instant.

Merging the anti-congestion algorithm before the release was a mistake and cost us more time, because we had some transport bugs coming up and needed to find out where it came from. Adding it back in will cost us even more time.

Revision history for this message
ypopezios (ypopezios) wrote :

It is funny how you still mention "this instance" when the scale of things here is whole months. Of course introducing new bugs doesn't speed-up things, but I don't remember you ever bothering with project's speed to begin with. Still introducing developers can definitely speed-up things. Especially those that are familiar with the introduced bugs, and those able for real development instead of merely hacking existing hacks. Which mistake was more serious in the longterm shouldn't be hard to see. The anti-congestion algorithm hardly introduced any bugs, it basically brought on the surface pre-existing ones. In any case, nobody cared for my time here, so I don't care if my contributions will be used or not. I have long moved on. Happy coding.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Yes, introducing new developers - especially ones that are as capable as you are - definitely does speed up things. Repeatedly spending a lot of time, energy and frustration on discussions like this one however does not.

And yes, development can always take longer that one has projected, which is the nature of things. Also, going into feature freeze is a common practice that you will need to learn how to deal with if you're still interested in working collaboratively on software projects this size, even if they are just for fun.

I'm sorry that we didn't manage to communicate better and that you have not been happy here. I wish you all the best and that you will find a community that will suit you better.

Revision history for this message
ypopezios (ypopezios) wrote :

What has been common practice when Widelands started development, is not the favorite approach anymore and doesn't have to remain the same practice forever. Big fails in prediction is not in the nature of programming, but in the nature of old-fashioned development. Collaborative development is not an exception. The industry has moved forward (although not all parts of it) and a project like Widelands would only benefit from reconsidering its paleolithic approach (and this is something that the rest of Widelands developers seemed to realize).

No, I don't need to deal with that stuff in our era. It's not my business anymore, but the ideal point in time to reconsider a project's approach is right after a major release, so don't lose that rare-in-Widelands opportunity. There are good chances that you will be wondering why you didn't do it earlier.

I have not been in search for a community, I didn't come here begging to belong somewhere. I offered to help, but I got "embraced" with immediate doubts and trash. I tried my best to consider it as a problem of communication or culture. So I gave it a chance and proved that I can contribute (which I did in various fields, including the most demanding one). In the end it was not a problem of communication, but one of realism. Even abandoning the project could be considered as a form of helping it wake up.

And I don't believe that I'm a special case. Not many developers would tolerate that environment. Widelands community is suffering from isolationism. Open doors cannot help when minds are closed. This is a well-known pattern in open-source projects.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/economy/fleet.cc'
2--- src/economy/fleet.cc 2019-02-23 11:00:49 +0000
3+++ src/economy/fleet.cc 2019-04-24 08:27:58 +0000
4@@ -305,7 +305,7 @@
5 *
6 * @return true if successful, or false if the docks are not actually part of the fleet.
7 */
8-bool Fleet::get_path(PortDock& start, PortDock& end, Path& path) {
9+bool Fleet::get_path(const PortDock& start, const PortDock& end, Path& path) {
10 uint32_t startidx = std::find(ports_.begin(), ports_.end(), &start) - ports_.begin();
11 uint32_t endidx = std::find(ports_.begin(), ports_.end(), &end) - ports_.begin();
12
13@@ -401,6 +401,7 @@
14 ship->set_economy(*game, nullptr);
15
16 if (ship->get_destination(egbase)) {
17+ ship->get_destination(egbase)->ship_coming(false);
18 update(egbase);
19 }
20
21@@ -633,8 +634,7 @@
22 }
23
24 /**
25- * Act callback updates ship scheduling. All decisions about where transport ships
26- * are supposed to go are made via this function.
27+ * Act callback updates ship scheduling of idle ships.
28 *
29 * @note Do not call this directly; instead, trigger it via @ref update
30 */
31@@ -642,230 +642,207 @@
32 act_pending_ = false;
33
34 if (!active()) {
35- // If we are here, most likely act() was called by a port with waiting wares or an expedition
36- // ready
37- // although there are still no ships. We can't handle it now, so we reschedule the act()
38- schedule_act(game, 5000); // retry in the next time
39+ // If we are here, most likely act() was called by a port with waiting wares or
40+ // with an expedition ready, although there are still no ships.
41+ // We can't handle it now, so we reschedule the act()
42+ schedule_act(game, kFleetInterval); // retry in the next time
43 act_pending_ = true;
44 return;
45 }
46
47 molog("Fleet::act\n");
48
49- // we need to calculate what ship is to be send to which port
50- // for this we will have temporary data structure with format
51- // <<ship,port>,score>
52- // where ship and port are not objects but positions in ports_ and ships_
53- // this is to allow native hashing
54- std::map<std::pair<uint16_t, uint16_t>, uint16_t> scores;
55-
56- // so we will identify all pairs: idle ship : ports, and score all such
57- // pairs. We consider
58- // - count of wares onboard, first ware (oldest) is counted as 8 (prioritization)
59- // (counting wares for particular port only)
60- // - count wares waiting at the port/3
61- // - distance between ship and a port (0-10 points, the closer the more points)
62- // - is another ship heading there right now?
63-
64- // at the end we must know if requrests of all ports asking for ship were addressed
65- // if any unsatisfied, we must schedule new run of this function
66- // when we send a ship there, the port is removed from list
67- std::list<uint16_t> waiting_ports;
68-
69- // this is just helper - first member of scores map
70- std::pair<uint16_t, uint16_t> mapping; // ship number, port number
71-
72- // first we go over ships - idle ones (=without destination)
73- // then over wares on these ships and create first ship-port
74- // pairs with score
75- for (uint16_t s = 0; s < ships_.size(); s += 1) {
76- if (ships_[s]->get_destination(game)) {
77- continue;
78- }
79- if (ships_[s]->get_ship_state() != Ship::ShipStates::kTransport) {
80- continue; // in expedition obviously
81- }
82-
83- for (uint16_t i = 0; i < ships_[s]->get_nritems(); i += 1) {
84- PortDock* dst = ships_[s]->items_[i].get_destination(game);
85- if (!dst) {
86- // if wares without destination on ship without destination
87- // such ship can be send to any port, and should be sent
88- // to some port, so we add 1 point to score for each port
89- for (uint16_t p = 0; p < ports_.size(); p += 1) {
90- mapping.first = s;
91- mapping.second = p;
92- scores[mapping] += 1;
93- }
94- continue;
95- }
96-
97- bool destination_found = false; // Just a functional check
98- for (uint16_t p = 0; p < ports_.size(); p += 1) {
99- if (ports_[p] == ships_[s]->items_[i].get_destination(game)) {
100- mapping.first = s;
101- mapping.second = p;
102- scores[mapping] += (i == 0) ? 8 : 1;
103- destination_found = true;
104- }
105- }
106- if (!destination_found) {
107- // Perhaps the throw here is too strong
108- // we can still remove it before stable release if it proves too much
109- // during my testing this situation never happened
110- throw wexception("A ware with destination that does not match any of player's"
111- " ports, ship %u, ware's destination: %u",
112- ships_[s]->serial(),
113- ships_[s]->items_[i].get_destination(game)->serial());
114- }
115- }
116- }
117-
118- // now opposite aproach - we go over ports to find out those that have wares
119- // waiting for ship then find candidate ships to satisfy the requests
120- for (uint16_t p = 0; p < ports_.size(); p += 1) {
121- PortDock& pd = *ports_[p];
122- if (!pd.get_need_ship()) {
123- continue;
124- }
125-
126- // general stategy is "one ship for port is enough", but sometimes
127- // amount of ware waiting for ship is too high
128- if (count_ships_heading_here(game, &pd) * 25 > pd.count_waiting()) {
129- continue;
130- }
131-
132- waiting_ports.push_back(p);
133-
134- // scoring and entering the pair into scores (or increasing existing
135- // score if the pair is already there)
136- for (uint16_t s = 0; s < ships_.size(); s += 1) {
137-
138- if (ships_[s]->get_destination(game)) {
139- continue; // already has destination
140- }
141-
142- if (ships_[s]->get_ship_state() != Ship::ShipStates::kTransport) {
143- continue; // in expedition obviously
144- }
145-
146- mapping.first = s;
147- mapping.second = p;
148- // following aproximately considers free capacity of a ship
149- scores[mapping] += ((ships_[s]->get_nritems() > 15) ? 1 : 3) +
150- std::min(ships_[s]->descr().get_capacity() - ships_[s]->get_nritems(),
151- ports_[p]->count_waiting()) /
152- 3;
153- }
154- }
155-
156- // Now adding score for distance
157- for (auto ship_port_relation : scores) {
158-
159- // here we get distance ship->port
160- // possibilities are:
161- // - we are in port and it is the same as target port
162- // - we are in other port, then we use get_dock() function to fetch precalculated path
163- // - if above fails, we calculate path "manually"
164- int16_t route_length = -1;
165-
166- PortDock* current_portdock =
167- get_dock(game, ships_[ship_port_relation.first.first]->get_position());
168-
169- if (current_portdock) { // we try to use precalculated paths of game
170-
171- // we are in the same portdock
172- if (current_portdock == ports_[ship_port_relation.first.second]) {
173- route_length = 0;
174- } else { // it is different portdock then
175- Path tmp_path;
176- if (get_path(*current_portdock, *ports_[ship_port_relation.first.second], tmp_path)) {
177- route_length = tmp_path.get_nsteps();
178- }
179- }
180- }
181-
182- // most probably the ship is not in a portdock (should not happen frequently)
183- if (route_length == -1) {
184- route_length = ships_[ship_port_relation.first.first]->calculate_sea_route(
185- game, *ports_[ship_port_relation.first.second]);
186- }
187-
188- // now we have length of route, so we need to calculate score
189- int16_t score_for_distance = 0;
190- if (route_length < 3) {
191- score_for_distance = 10;
192- } else {
193- score_for_distance = 8 - route_length / 50;
194- }
195- // must not be negative
196- score_for_distance = (score_for_distance < 0) ? 0 : score_for_distance;
197-
198- scores[ship_port_relation.first] += score_for_distance;
199- }
200-
201- // looking for best scores and sending ships accordingly
202- uint16_t best_ship = 0;
203- uint16_t best_port = 0;
204-
205- // after sending a ship we will remove one or more items from scores
206- while (!scores.empty()) {
207- uint16_t best_score = 0;
208-
209- // searching for combination with highest score
210- for (const auto& combination : scores) {
211- if (combination.second > best_score) {
212- best_score = combination.second;
213- best_ship = combination.first.first;
214- best_port = combination.first.second;
215- }
216- }
217- if (best_score == 0) {
218- // this is check of correctnes of this algorithm, this should not happen
219- throw wexception("Fleet::act(): No port-destination pair selected or its score is zero");
220- }
221-
222- // making sure the winner has no destination set
223- assert(!ships_[best_ship]->get_destination(game));
224-
225- // now actual setting destination for "best ship"
226- ships_[best_ship]->set_destination(game, *ports_[best_port]);
227- molog("... ship %u sent to port %u, wares onboard: %2d, the port is asking for a ship: %s\n",
228- ships_[best_ship]->serial(), ports_[best_port]->serial(),
229- ships_[best_ship]->get_nritems(), (ports_[best_port]->get_need_ship()) ? "yes" : "no");
230-
231- // pruning the scores table
232- // the ship that was just sent somewhere cannot be send elsewhere :)
233- for (auto it = scores.cbegin(); it != scores.cend();) {
234-
235- // decreasing score for target port as there was a ship just sent there
236- if (it->first.second == best_port) {
237- mapping.first = it->first.first;
238- mapping.second = it->first.second;
239- scores[mapping] /= 2;
240- // just make sure it is nonzero
241- scores[mapping] = (scores[mapping] == 0) ? 1 : scores[mapping];
242- }
243-
244- // but removing all pairs where best ship is participating as it is not available anymore
245- // (because it was sent to "best port")
246- if (it->first.first == best_ship) {
247- scores.erase(it++);
248- } else {
249- ++it;
250- }
251- }
252-
253- // also removing the port from waiting_ports
254- waiting_ports.remove(best_port);
255- }
256-
257- if (!waiting_ports.empty()) {
258- molog("... there are %" PRIuS " ports requesting ship(s) we cannot satisfy yet\n",
259- waiting_ports.size());
260- schedule_act(game, 5000); // retry next time
261+ // For each waiting port, try to find idle ships and send to it the closest one.
262+ uint16_t waiting_ports = ports_.size();
263+ for (PortDock* p : ports_) {
264+ if (p->get_need_ship() == 0) {
265+ --waiting_ports;
266+ continue;
267+ }
268+
269+ Ship* closest_ship = nullptr;
270+ uint32_t shortest_dist = kRouteNotCalculated;
271+ bool waiting = true;
272+
273+ for (Ship* s : ships_) {
274+ if (s->get_destination(game)) {
275+ if (s->get_destination(game) == p) {
276+ waiting = false;
277+ --waiting_ports;
278+ break;
279+ }
280+ continue; // The ship already has a destination
281+ }
282+ if (s->get_ship_state() != Ship::ShipStates::kTransport) {
283+ continue; // Ship is not available, e.g. in expedition
284+ }
285+
286+ // Here we get distance ship->port
287+ uint32_t route_length = kRouteNotCalculated;
288+
289+ // Get precalculated distance for ships available at ports
290+ {
291+ PortDock* cur_port = get_dock(game, s->get_position());
292+ if (cur_port) { // Ship is at a port
293+ if (cur_port == p) { // Same port
294+ route_length = 0;
295+ } else { // Different port
296+ Path precalculated_path;
297+ if (get_path(*cur_port, *p, precalculated_path)) {
298+ route_length = precalculated_path.get_nsteps();
299+ }
300+ }
301+ }
302+ }
303+
304+ // Get distance for ships available but not at a port (should not happen frequently)
305+ if (route_length == kRouteNotCalculated) {
306+ route_length = s->calculate_sea_route(game, *p);
307+ }
308+
309+ if (route_length < shortest_dist) {
310+ shortest_dist = route_length;
311+ closest_ship = s;
312+ }
313+ }
314+
315+ if (waiting && closest_ship) {
316+ --waiting_ports;
317+ closest_ship->set_destination(p);
318+ closest_ship->send_signal(game, "wakeup");
319+ }
320+ }
321+
322+ if (waiting_ports > 0) {
323+ molog("... there are %u ports requesting ship(s) we cannot satisfy yet\n", waiting_ports);
324+ schedule_act(game, kFleetInterval); // retry next time
325 act_pending_ = true;
326 }
327+
328+ // Deal with edge-case of losing destination before reaching it
329+ for (Ship* s : ships_) {
330+ if (s->get_destination(game)) {
331+ continue; // The ship has a destination
332+ }
333+ if (s->get_ship_state() != Ship::ShipStates::kTransport) {
334+ continue; // Ship is not available, e.g. in expedition
335+ }
336+ if (s->items_.empty()) {
337+ continue; // No pending wares/workers
338+ }
339+
340+ // Send ship to the closest port
341+ PortDock* closest_port = nullptr;
342+ uint32_t shortest_dist = kRouteNotCalculated;
343+
344+ for (PortDock* p : ports_) {
345+ uint32_t route_length = s->calculate_sea_route(game, *p);
346+ if (route_length < shortest_dist) {
347+ shortest_dist = route_length;
348+ closest_port = p;
349+ }
350+ }
351+
352+ if (closest_port) {
353+ s->set_destination(closest_port);
354+ s->send_signal(game, "wakeup");
355+ }
356+ }
357+}
358+
359+/**
360+ * For the given three consecutive ports, decide if their path is favourable or not.
361+ * \return true if the path from start to finish >= the path from middle to finish
362+ */
363+bool Fleet::is_path_favourable(const PortDock& start, const PortDock& middle, const PortDock& finish) {
364+ if (&middle != &finish) {
365+ Path path_start_to_finish;
366+ Path path_middle_to_finish;
367+#ifndef NDEBUG
368+ assert(get_path(start, finish, path_start_to_finish));
369+#else
370+ get_path(start, finish, path_start_to_finish);
371+#endif
372+ if (get_path(middle, finish, path_middle_to_finish)) {
373+ if (path_middle_to_finish.get_nsteps() > path_start_to_finish.get_nsteps()) {
374+ return false;
375+ }
376+ }
377+ }
378+ return true; // default
379+}
380+
381+/**
382+ * For the given ship, go through all ports of this fleet
383+ * and find the one with the best score.
384+ * \return that port
385+ */
386+PortDock* Fleet::find_next_dest(Game& game, const Ship& ship, const PortDock& from_port) {
387+ PortDock* best_port = nullptr;
388+ float best_score = 0.0f;
389+
390+ for (PortDock* p : ports_) {
391+ if (p == &from_port) {
392+ continue; // same port
393+ }
394+
395+ float score = 0.0f;
396+ WareInstance* ware;
397+ Worker* worker;
398+
399+ // Score for wares/workers onboard that ship for that port
400+ for (const ShippingItem& si : ship.items_) {
401+ if (si.get_destination(game) == p) {
402+ si.get(game, &ware, &worker);
403+ if (ware) {
404+ score += 1; // TODO(ypopezios): increase by ware's importance
405+ } else { // worker
406+ score += 4;
407+ }
408+ }
409+ }
410+
411+ // Score for wares/workers waiting at that port
412+ for (const ShippingItem& si : from_port.waiting_) {
413+ if (si.get_destination(game) == p) {
414+ si.get(game, &ware, &worker);
415+ if (ware) {
416+ score += 1; // TODO(ypopezios): increase by ware's importance
417+ } else { // worker
418+ score += 4;
419+ }
420+ }
421+ }
422+
423+ if (score == 0.0f && p->get_need_ship() == 0) {
424+ continue; // empty ship to empty port
425+ }
426+
427+ // Here we get distance ship->port
428+ uint32_t route_length = kRouteNotCalculated;
429+
430+ // Get precalculated distance if the ship is at a port
431+ {
432+ Path precalculated_path;
433+ if (get_path(from_port, *p, precalculated_path)) { // try to use precalculated path
434+ route_length = precalculated_path.get_nsteps();
435+ }
436+ }
437+
438+ // Get distance for when the ship is not at a port (should not happen frequently)
439+ if (route_length == kRouteNotCalculated) {
440+ route_length = ship.calculate_sea_route(game, *p);
441+ }
442+
443+ score = (score + 1.0f) * (score + p->get_need_ship());
444+ score = score * (1.0f - route_length / (score + route_length));
445+ if (score > best_score) {
446+ best_score = score;
447+ best_port = p;
448+ }
449+ }
450+
451+ return best_port;
452 }
453
454 void Fleet::log_general_info(const EditorGameBase& egbase) const {
455
456=== modified file 'src/economy/fleet.h'
457--- src/economy/fleet.h 2019-02-23 11:00:49 +0000
458+++ src/economy/fleet.h 2019-04-24 08:27:58 +0000
459@@ -46,6 +46,9 @@
460 DISALLOW_COPY_AND_ASSIGN(FleetDescr);
461 };
462
463+constexpr int32_t kFleetInterval = 5000;
464+constexpr uint32_t kRouteNotCalculated = std::numeric_limits<uint32_t>::max();
465+
466 /**
467 * Manage all ships and ports of a player that are connected
468 * by ocean.
469@@ -96,7 +99,7 @@
470
471 void log_general_info(const EditorGameBase&) const override;
472
473- bool get_path(PortDock& start, PortDock& end, Path& path);
474+ bool get_path(const PortDock& start, const PortDock& end, Path& path);
475 void add_neighbours(PortDock& pd, std::vector<RoutingNodeNeighbour>& neighbours);
476
477 uint32_t count_ships() const;
478@@ -152,6 +155,8 @@
479 void save(EditorGameBase&, MapObjectSaver&, FileWrite&) override;
480
481 static MapObject::Loader* load(EditorGameBase&, MapObjectLoader&, FileRead&);
482+ bool is_path_favourable(const PortDock& start, const PortDock& middle, const PortDock& finish);
483+ PortDock* find_next_dest(Game&, const Ship&, const PortDock& from_port);
484 };
485
486 } // namespace Widelands
487
488=== modified file 'src/economy/portdock.cc'
489--- src/economy/portdock.cc 2019-04-24 06:01:37 +0000
490+++ src/economy/portdock.cc 2019-04-24 08:27:58 +0000
491@@ -34,6 +34,7 @@
492 #include "logic/game_data_error.h"
493 #include "logic/map_objects/tribes/ship.h"
494 #include "logic/map_objects/tribes/warehouse.h"
495+#include "logic/path.h"
496 #include "logic/player.h"
497 #include "logic/widelands_geometry_io.h"
498 #include "map_io/map_object_loader.h"
499@@ -55,7 +56,7 @@
500 : PlayerImmovable(g_portdock_descr),
501 fleet_(nullptr),
502 warehouse_(wh),
503- need_ship_(false),
504+ ships_coming_(0),
505 expedition_ready_(false) {
506 }
507
508@@ -116,6 +117,10 @@
509 return nullptr;
510 }
511
512+uint32_t PortDock::get_need_ship() const {
513+ return (waiting_.size() + (expedition_ready_ ? 20 : 0)) / (ships_coming_ + 1);
514+}
515+
516 /**
517 * Signal to the dock that it now belongs to the given economy.
518 *
519@@ -248,7 +253,7 @@
520 * its route.
521 */
522 void PortDock::update_shippingitem(Game& game, WareInstance& ware) {
523- for (std::vector<ShippingItem>::iterator item_iter = waiting_.begin();
524+ for (auto item_iter = waiting_.begin();
525 item_iter != waiting_.end(); ++item_iter) {
526
527 if (item_iter->object_.serial() == ware.serial()) {
528@@ -272,7 +277,7 @@
529 * updated its route.
530 */
531 void PortDock::update_shippingitem(Game& game, Worker& worker) {
532- for (std::vector<ShippingItem>::iterator item_iter = waiting_.begin();
533+ for (auto item_iter = waiting_.begin();
534 item_iter != waiting_.end(); ++item_iter) {
535
536 if (item_iter->object_.serial() == worker.serial()) {
537@@ -282,44 +287,66 @@
538 }
539 }
540
541-void PortDock::update_shippingitem(Game& game, std::vector<ShippingItem>::iterator it) {
542+void PortDock::update_shippingitem(Game& game, std::list<ShippingItem>::iterator it) {
543 it->update_destination(game, *this);
544
545- PortDock* dst = it->get_destination(game);
546+ const PortDock* dst = it->get_destination(game);
547 assert(dst != this);
548
549 // Destination might have vanished or be in another economy altogether.
550 if (dst && dst->get_economy() == get_economy()) {
551- set_need_ship(game, true);
552+ if (ships_coming_ <= 0) {
553+ set_need_ship(game, true);
554+ }
555 } else {
556 it->set_location(game, warehouse_);
557 it->end_shipping(game);
558 *it = waiting_.back();
559 waiting_.pop_back();
560-
561- if (waiting_.empty())
562- set_need_ship(game, false);
563- }
564-}
565-
566-/**
567- * A ship has arrived at the dock. Clear all items designated for this dock,
568- * and load the ship.
569+ }
570+}
571+
572+/**
573+ * Receive shipping item from unloading ship.
574+ * Called by ship code.
575+ */
576+void PortDock::shipping_item_arrived(Game& game, ShippingItem& si) {
577+ si.set_location(game, warehouse_);
578+ si.end_shipping(game);
579+}
580+
581+/**
582+ * Receive shipping item from departing ship.
583+ * Called by ship code.
584+ */
585+void PortDock::shipping_item_returned(Game& game, ShippingItem& si) {
586+ si.set_location(game, this);
587+ waiting_.push_back(si);
588+}
589+
590+/**
591+ * A ship changed destination and is now coming to the dock. Increase counter for need_ship.
592+ */
593+void PortDock::ship_coming(bool affirmative) {
594+ if (affirmative) {
595+ ++ships_coming_;
596+ } else {
597+ --ships_coming_;
598+ }
599+}
600+
601+/**
602+ * A ship has arrived at the dock. Set its next destination and load it accordingly.
603 */
604 void PortDock::ship_arrived(Game& game, Ship& ship) {
605- std::vector<ShippingItem> items_brought_by_ship;
606- ship.withdraw_items(game, *this, items_brought_by_ship);
607-
608- for (ShippingItem& shipping_item : items_brought_by_ship) {
609- shipping_item.set_location(game, warehouse_);
610- shipping_item.end_shipping(game);
611- }
612+ ship_coming(false);
613
614 if (expedition_ready_) {
615 assert(expedition_bootstrap_ != nullptr);
616
617 // Only use an empty ship.
618 if (ship.get_nritems() < 1) {
619+ ship.set_destination(nullptr);
620 // Load the ship
621 std::vector<Worker*> workers;
622 std::vector<WareInstance*> wares;
623@@ -338,46 +365,59 @@
624 // The expedition goods are now on the ship, so from now on it is independent from the port
625 // and thus we switch the port to normal, so we could even start a new expedition,
626 cancel_expedition(game);
627- return fleet_->update(game);
628- }
629- }
630-
631- if (ship.get_nritems() < ship.descr().get_capacity() && !waiting_.empty()) {
632- uint32_t nrload =
633- std::min<uint32_t>(waiting_.size(), ship.descr().get_capacity() - ship.get_nritems());
634-
635- while (nrload--) {
636- // Check if the item has still a valid destination
637- if (waiting_.back().get_destination(game)) {
638- // Destination is valid, so we load the item onto the ship
639- ship.add_item(game, waiting_.back());
640- } else {
641- // The item has no valid destination anymore, so we just carry it
642- // back in the warehouse
643- waiting_.back().set_location(game, warehouse_);
644- waiting_.back().end_shipping(game);
645- }
646- waiting_.pop_back();
647- }
648-
649- if (waiting_.empty()) {
650- set_need_ship(game, false);
651- }
652- }
653-
654- fleet_->update(game);
655+ fleet_->update(game);
656+ return;
657+ }
658+ }
659+
660+ // Check for items with invalid destination. TODO(ypopezios): Prevent invalid destinations
661+ for (auto si_it = waiting_.begin(); si_it != waiting_.end(); ++si_it) {
662+ if (!si_it->get_destination(game)) {
663+ // Invalid destination. Carry the item back into the warehouse
664+ si_it->set_location(game, warehouse_);
665+ si_it->end_shipping(game);
666+ si_it = waiting_.erase(si_it);
667+ }
668+ }
669+
670+ // Decide where the arrived ship will go next
671+ PortDock* next_port = fleet_->find_next_dest(game, ship, *this);
672+ if (!next_port) {
673+ ship.set_destination(next_port);
674+ return; // no need to load anything
675+ }
676+
677+ // Unload any wares/workers onboard the departing ship which are not favored by next dest
678+ ship.unload_unfit_items(game, *this, *next_port);
679+
680+ // Then load the remaining capacity of the departing ship with relevant items
681+ uint32_t remaining_capacity = ship.descr().get_capacity() - ship.get_nritems();
682+
683+ // Firstly load the wares/workers which go to chosen destination
684+ for (auto si_it = waiting_.begin(); si_it != waiting_.end() && remaining_capacity > 0; ++si_it) {
685+ if (si_it->get_destination(game) == next_port) {
686+ ship.add_item(game, *si_it);
687+ si_it = waiting_.erase(si_it);
688+ --remaining_capacity;
689+ }
690+ }
691+
692+ // Then load any wares/workers favored by the chosen destination
693+ for (auto si_it = waiting_.begin(); si_it != waiting_.end() && remaining_capacity > 0; ++si_it) {
694+ if (fleet_->is_path_favourable(*this, *next_port, *si_it->get_destination(game))) {
695+ ship.add_item(game, *si_it);
696+ si_it = waiting_.erase(si_it);
697+ --remaining_capacity;
698+ }
699+ }
700+
701+ ship.set_destination(next_port);
702+ set_need_ship(game, !waiting_.empty());
703 }
704
705 void PortDock::set_need_ship(Game& game, bool need) {
706- molog("set_need_ship(%s)\n", need ? "true" : "false");
707-
708- if (need == need_ship_)
709- return;
710-
711- need_ship_ = need;
712-
713- if (fleet_) {
714- molog("... trigger fleet update\n");
715+ if (need && fleet_) {
716+ molog("trigger fleet update\n");
717 fleet_->update(game);
718 }
719 }
720@@ -446,13 +486,13 @@
721
722 if (warehouse_) {
723 Coords pos(warehouse_->get_position());
724- molog("PortDock for warehouse %u (at %i,%i) in fleet %u, need_ship: %s, waiting: %" PRIuS
725+ molog("PortDock for warehouse %u (at %i,%i) in fleet %u, expedition_ready: %s, waiting: %" PRIuS
726 "\n",
727 warehouse_->serial(), pos.x, pos.y, fleet_ ? fleet_->serial() : 0,
728- need_ship_ ? "true" : "false", waiting_.size());
729+ expedition_ready_ ? "true" : "false", waiting_.size());
730 } else {
731- molog("PortDock without a warehouse in fleet %u, need_ship: %s, waiting: %" PRIuS "\n",
732- fleet_ ? fleet_->serial() : 0, need_ship_ ? "true" : "false", waiting_.size());
733+ molog("PortDock without a warehouse in fleet %u, expedition_ready: %s, waiting: %" PRIuS "\n",
734+ fleet_ ? fleet_->serial() : 0, expedition_ready_ ? "true" : "false", waiting_.size());
735 }
736
737 for (const ShippingItem& shipping_item : waiting_) {
738@@ -461,12 +501,12 @@
739 }
740 }
741
742-constexpr uint8_t kCurrentPacketVersion = 3;
743+constexpr uint8_t kCurrentPacketVersion = 4;
744
745 PortDock::Loader::Loader() : warehouse_(0) {
746 }
747
748-void PortDock::Loader::load(FileRead& fr) {
749+void PortDock::Loader::load(FileRead& fr, uint8_t packet_version) {
750 PlayerImmovable::Loader::load(fr);
751
752 PortDock& pd = get<PortDock>();
753@@ -480,7 +520,18 @@
754 pd.set_position(egbase(), pd.dockpoints_[i]);
755 }
756
757- pd.need_ship_ = fr.unsigned_8();
758+ pd.ships_coming_ = fr.unsigned_8();
759+
760+ // TODO(GunChleoc): Savegame compatibility Build 20
761+ if (packet_version < 4) {
762+ pd.ships_coming_ = 0;
763+ for (const Serial ship_serial : pd.owner().ships()) {
764+ Ship* ship = dynamic_cast<Ship*>(egbase().objects().get_object(ship_serial));
765+ if (ship->get_destination(egbase())->serial() == pd.serial()) {
766+ ++pd.ships_coming_;
767+ }
768+ }
769+ }
770
771 waiting_.resize(fr.unsigned_32());
772 for (ShippingItem::Loader& shipping_loader : waiting_) {
773@@ -500,10 +551,10 @@
774 PortDock& pd = get<PortDock>();
775 pd.warehouse_ = &mol().get<Warehouse>(warehouse_);
776
777- pd.waiting_.resize(waiting_.size());
778 for (uint32_t i = 0; i < waiting_.size(); ++i) {
779- pd.waiting_[i] = waiting_[i].get(mol());
780+ pd.waiting_.push_back(waiting_[i].get(mol()));
781 }
782+ assert(pd.waiting_.size() == waiting_.size());
783 }
784
785 void PortDock::Loader::load_finish() {
786@@ -528,10 +579,11 @@
787 try {
788 // The header has been peeled away by the caller
789
790+ // TODO(GunChleoc): Savegame compatibility Build 20
791 uint8_t const packet_version = fr.unsigned_8();
792- if (packet_version == kCurrentPacketVersion) {
793+ if (packet_version >= 3 && packet_version <= kCurrentPacketVersion) {
794 loader->init(egbase, mol, *new PortDock(nullptr));
795- loader->load(fr);
796+ loader->load(fr, packet_version);
797 } else {
798 throw UnhandledVersionError("PortDock", packet_version, kCurrentPacketVersion);
799 }
800@@ -554,7 +606,7 @@
801 write_coords_32(&fw, coords);
802 }
803
804- fw.unsigned_8(need_ship_);
805+ fw.unsigned_8(ships_coming_);
806
807 fw.unsigned_32(waiting_.size());
808 for (ShippingItem& shipping_item : waiting_) {
809
810=== modified file 'src/economy/portdock.h'
811--- src/economy/portdock.h 2019-04-24 06:01:37 +0000
812+++ src/economy/portdock.h 2019-04-24 08:27:58 +0000
813@@ -20,6 +20,7 @@
814 #ifndef WL_ECONOMY_PORTDOCK_H
815 #define WL_ECONOMY_PORTDOCK_H
816
817+#include <list>
818 #include <memory>
819
820 #include "base/macros.h"
821@@ -85,9 +86,7 @@
822 return fleet_;
823 }
824 PortDock* get_dock(Flag& flag) const;
825- bool get_need_ship() const {
826- return need_ship_ || expedition_ready_;
827- }
828+ uint32_t get_need_ship() const;
829
830 void set_economy(Economy*) override;
831
832@@ -114,6 +113,9 @@
833 void add_shippingitem(Game&, Worker&);
834 void update_shippingitem(Game&, Worker&);
835
836+ void shipping_item_arrived(Game&, ShippingItem&);
837+ void shipping_item_returned(Game&, ShippingItem&);
838+ void ship_coming(bool affirmative);
839 void ship_arrived(Game&, Ship&);
840
841 void log_general_info(const EditorGameBase&) const override;
842@@ -140,14 +142,14 @@
843
844 void init_fleet(EditorGameBase& egbase);
845 void set_fleet(Fleet* fleet);
846- void update_shippingitem(Game&, std::vector<ShippingItem>::iterator);
847+ void update_shippingitem(Game&, std::list<ShippingItem>::iterator);
848 void set_need_ship(Game&, bool need);
849
850 Fleet* fleet_;
851 Warehouse* warehouse_;
852 PositionList dockpoints_;
853- std::vector<ShippingItem> waiting_;
854- bool need_ship_;
855+ std::list<ShippingItem> waiting_;
856+ uint8_t ships_coming_;
857 bool expedition_ready_;
858
859 std::unique_ptr<ExpeditionBootstrap> expedition_bootstrap_;
860@@ -158,7 +160,7 @@
861 public:
862 Loader();
863
864- void load(FileRead&);
865+ void load(FileRead&, uint8_t packet_version);
866 void load_pointers() override;
867 void load_finish() override;
868
869
870=== modified file 'src/economy/shippingitem.cc'
871--- src/economy/shippingitem.cc 2019-02-23 11:00:49 +0000
872+++ src/economy/shippingitem.cc 2019-04-24 08:27:58 +0000
873@@ -105,7 +105,7 @@
874 worker->end_shipping(game);
875 }
876
877-PortDock* ShippingItem::get_destination(Game& game) {
878+const PortDock* ShippingItem::get_destination(Game& game) const {
879 return destination_dock_.get(game);
880 }
881
882
883=== modified file 'src/economy/shippingitem.h'
884--- src/economy/shippingitem.h 2019-02-23 11:00:49 +0000
885+++ src/economy/shippingitem.h 2019-04-24 08:27:58 +0000
886@@ -53,7 +53,7 @@
887 void get(const EditorGameBase& game, WareInstance** ware, Worker** worker) const;
888
889 void set_economy(Game&, Economy* e);
890- PortDock* get_destination(Game&);
891+ const PortDock* get_destination(Game&) const;
892
893 void remove(EditorGameBase&);
894
895
896=== modified file 'src/logic/map_objects/tribes/ship.cc'
897--- src/logic/map_objects/tribes/ship.cc 2019-04-24 06:01:37 +0000
898+++ src/logic/map_objects/tribes/ship.cc 2019-04-24 08:27:58 +0000
899@@ -303,12 +303,22 @@
900
901 FCoords position = map.get_fcoords(get_position());
902 if (position.field->get_immovable() == dst) {
903- molog("ship_update: Arrived at dock %u\n", dst->serial());
904- lastdock_ = dst;
905- destination_ = nullptr;
906- dst->ship_arrived(game, *this);
907- start_task_idle(game, descr().main_animation(), 250);
908- Notifications::publish(NoteShip(this, NoteShip::Action::kDestinationChanged));
909+ if (lastdock_ != dst) {
910+ molog("ship_update: Arrived at dock %u\n", dst->serial());
911+ lastdock_ = dst;
912+ }
913+ if (withdraw_item(game, *dst)) {
914+ schedule_act(game, kShipInterval);
915+ return true;
916+ }
917+
918+ dst->ship_arrived(game, *this); // This will also set the destination
919+ dst = get_destination(game);
920+ if (dst) {
921+ start_task_movetodock(game, *dst);
922+ } else {
923+ start_task_idle(game, descr().main_animation(), 250);
924+ }
925 return true;
926 }
927
928@@ -484,7 +494,7 @@
929 }
930
931 if (totalprob == 0) {
932- start_task_idle(game, descr().main_animation(), 1500);
933+ start_task_idle(game, descr().main_animation(), kShipInterval);
934 return;
935 }
936
937@@ -496,13 +506,13 @@
938 }
939
940 if (dir == 0 || dir > LAST_DIRECTION) {
941- start_task_idle(game, descr().main_animation(), 1500);
942+ start_task_idle(game, descr().main_animation(), kShipInterval);
943 return;
944 }
945
946 FCoords neighbour = map.get_neighbour(position, dir);
947 if (!(neighbour.field->nodecaps() & MOVECAPS_SWIM)) {
948- start_task_idle(game, descr().main_animation(), 1500);
949+ start_task_idle(game, descr().main_animation(), kShipInterval);
950 return;
951 }
952
953@@ -542,7 +552,7 @@
954 pgettext("ship", "Waiting"), _("Island Circumnavigated"),
955 _("An expedition ship sailed around its island without any events."),
956 "images/wui/ship/ship_explore_island_cw.png");
957- return start_task_idle(game, descr().main_animation(), 1500);
958+ return start_task_idle(game, descr().main_animation(), kShipInterval);
959 }
960 }
961 // The ship is supposed to follow the coast as close as possible, therefore the check
962@@ -585,7 +595,7 @@
963 shipname_.c_str());
964 set_ship_state_and_notify(
965 ShipStates::kExpeditionWaiting, NoteShip::Action::kWaitingForCommand);
966- start_task_idle(game, descr().main_animation(), 1500);
967+ start_task_idle(game, descr().main_animation(), kShipInterval);
968 return;
969 }
970 } else { // scouting towards a specific direction
971@@ -598,7 +608,7 @@
972 // coast reached
973 set_ship_state_and_notify(
974 ShipStates::kExpeditionWaiting, NoteShip::Action::kWaitingForCommand);
975- start_task_idle(game, descr().main_animation(), 1500);
976+ start_task_idle(game, descr().main_animation(), kShipInterval);
977 // Send a message to the player, that a new coast was reached
978 send_message(game,
979 /** TRANSLATORS: A ship has discovered land */
980@@ -692,14 +702,14 @@
981 }
982
983 expedition_.reset(nullptr);
984- return start_task_idle(game, descr().main_animation(), 1500);
985+ return start_task_idle(game, descr().main_animation(), kShipInterval);
986 }
987 }
988 FALLS_THROUGH;
989 case ShipStates::kExpeditionWaiting:
990 case ShipStates::kExpeditionPortspaceFound: {
991 // wait for input
992- start_task_idle(game, descr().main_animation(), 1500);
993+ start_task_idle(game, descr().main_animation(), kShipInterval);
994 return;
995 }
996 case ShipStates::kSinkRequest:
997@@ -728,14 +738,17 @@
998
999 /**
1000 * Enter a new destination port for the ship.
1001- *
1002- * @note This is supposed to be called only from the scheduling code of @ref Fleet.
1003+ * Call this after (un)loading the ship, for proper logging.
1004 */
1005-void Ship::set_destination(Game& game, PortDock& pd) {
1006- molog("set_destination / sending to portdock %u (carrying %" PRIuS " items)\n", pd.serial(),
1007- items_.size());
1008- destination_ = &pd;
1009- send_signal(game, "wakeup");
1010+void Ship::set_destination(PortDock* pd) {
1011+ destination_ = pd;
1012+ if (pd) {
1013+ molog("set_destination / sending to portdock %u (carrying %" PRIuS " items)\n", pd->serial(),
1014+ items_.size());
1015+ pd->ship_coming(true);
1016+ } else {
1017+ molog("set_destination / none\n");
1018+ }
1019 Notifications::publish(NoteShip(this, NoteShip::Action::kDestinationChanged));
1020 }
1021
1022@@ -746,14 +759,39 @@
1023 items_.back().set_location(game, this);
1024 }
1025
1026-void Ship::withdraw_items(Game& game, PortDock& pd, std::vector<ShippingItem>& items) {
1027- uint32_t dst = 0;
1028- for (uint32_t src = 0; src < items_.size(); ++src) {
1029- PortDock* destination = items_[src].get_destination(game);
1030- if (!destination || destination == &pd) {
1031- items.push_back(items_[src]);
1032+/**
1033+ * Unload one item designated for given dock or for no dock.
1034+ * \return true if item unloaded.
1035+ */
1036+bool Ship::withdraw_item(Game& game, PortDock& pd) {
1037+ bool unloaded = false;
1038+ size_t dst = 0;
1039+ for (ShippingItem& si : items_) {
1040+ if (!unloaded) {
1041+ const PortDock* itemdest = si.get_destination(game);
1042+ if (!itemdest || itemdest == &pd) {
1043+ pd.shipping_item_arrived(game, si);
1044+ unloaded = true;
1045+ continue;
1046+ }
1047+ }
1048+ items_[dst++] = si;
1049+ }
1050+ items_.resize(dst);
1051+ return unloaded;
1052+}
1053+
1054+/**
1055+ * Unload all items not favored by given next dest.
1056+ * Assert all items for current portdock have already been unloaded.
1057+ */
1058+void Ship::unload_unfit_items(Game& game, PortDock& here, const PortDock& nextdest) {
1059+ size_t dst = 0;
1060+ for (ShippingItem& si : items_) {
1061+ if (fleet_->is_path_favourable(here, nextdest, *si.get_destination(game))) {
1062+ items_[dst++] = si;
1063 } else {
1064- items_[dst++] = items_[src];
1065+ here.shipping_item_returned(game, si);
1066 }
1067 }
1068 items_.resize(dst);
1069@@ -812,7 +850,7 @@
1070 // I (tiborb) failed to invoke this situation when testing so
1071 // I am not sure if following line behaves allright
1072 get_fleet()->update(game);
1073- start_task_idle(game, descr().main_animation(), 5000);
1074+ start_task_idle(game, descr().main_animation(), kFleetInterval);
1075 }
1076 }
1077
1078@@ -836,10 +874,10 @@
1079
1080 set_economy(game, expedition_->economy);
1081
1082- for (int i = items_.size() - 1; i >= 0; --i) {
1083+ for (const ShippingItem& si : items_) {
1084 WareInstance* ware;
1085 Worker* worker;
1086- items_.at(i).get(game, &ware, &worker);
1087+ si.get(game, &ware, &worker);
1088 if (worker) {
1089 worker->reset_tasks(game);
1090 worker->start_task_idle(game, 0, -1);
1091@@ -970,8 +1008,9 @@
1092 /// @note only called via player command
1093 void Ship::sink_ship(Game& game) {
1094 // Running colonization has the highest priority + a sink request is only valid once
1095- if (!state_is_sinkable())
1096+ if (!state_is_sinkable()) {
1097 return;
1098+ }
1099 ship_state_ = ShipStates::kSinkRequest;
1100 // Make sure the ship is active and close possible open windows
1101 ship_wakeup(game);
1102
1103=== modified file 'src/logic/map_objects/tribes/ship.h'
1104--- src/logic/map_objects/tribes/ship.h 2019-04-24 06:01:37 +0000
1105+++ src/logic/map_objects/tribes/ship.h 2019-04-24 08:27:58 +0000
1106@@ -78,6 +78,8 @@
1107 DISALLOW_COPY_AND_ASSIGN(ShipDescr);
1108 };
1109
1110+constexpr int32_t kShipInterval = 1500;
1111+
1112 /**
1113 * Ships belong to a player and to an economy. The usually are in a (unique)
1114 * fleet for a player, but only if they are on standard duty. Exploration ships
1115@@ -104,7 +106,7 @@
1116 return economy_;
1117 }
1118 void set_economy(Game&, Economy* e);
1119- void set_destination(Game&, PortDock&);
1120+ void set_destination(PortDock*);
1121
1122 void init_auto_task(Game&) override;
1123
1124@@ -126,8 +128,9 @@
1125 return items_[idx];
1126 }
1127
1128- void withdraw_items(Game& game, PortDock& pd, std::vector<ShippingItem>& items);
1129- void add_item(Game&, const ShippingItem& item);
1130+ void add_item(Game&, const ShippingItem&);
1131+ bool withdraw_item(Game&, PortDock&);
1132+ void unload_unfit_items(Game&, PortDock& here, const PortDock& nextdest);
1133
1134 // A ship with task expedition can be in four states: kExpeditionWaiting, kExpeditionScouting,
1135 // kExpeditionPortspaceFound or kExpeditionColonizing in the first states, the owning player of
1136
1137=== modified file 'test/maps/expedition.wmf/scripting/init.lua'
1138--- test/maps/expedition.wmf/scripting/init.lua 2017-11-08 15:53:57 +0000
1139+++ test/maps/expedition.wmf/scripting/init.lua 2019-04-24 08:27:58 +0000
1140@@ -207,7 +207,7 @@
1141
1142 -- Now cancel the expedition before it even got send out.
1143 cancel_expedition_in_shipwindow()
1144- sleep(100)
1145+ sleep(40000)
1146 assert_equal(1, p1:get_workers("barbarians_builder"))
1147 sleep(8000) -- ship needs a while to get wares back.
1148 check_wares_in_port_are_all_there()
1149@@ -248,7 +248,7 @@
1150 assert_equal(1, p1:get_workers("barbarians_builder"))
1151
1152 cancel_expedition_in_shipwindow(expedition_ship)
1153- sleep(20000)
1154+ sleep(50000)
1155 assert_equal(1, p1:get_workers("barbarians_builder"))
1156 check_wares_in_port_are_all_there()
1157
1158@@ -288,7 +288,7 @@
1159 assert_equal(1, p1:get_workers("barbarians_builder"))
1160
1161 cancel_expedition_in_shipwindow(first_ship)
1162- sleep(20000)
1163+ sleep(50000)
1164 assert_equal(1, p1:get_workers("barbarians_builder"))
1165 check_wares_in_port_are_all_there()
1166
1167
1168=== modified file 'test/maps/expedition.wmf/scripting/test_cancel_when_port_space_was_reached_two_ships.lua'
1169--- test/maps/expedition.wmf/scripting/test_cancel_when_port_space_was_reached_two_ships.lua 2017-11-08 15:53:57 +0000
1170+++ test/maps/expedition.wmf/scripting/test_cancel_when_port_space_was_reached_two_ships.lua 2019-04-24 08:27:58 +0000
1171@@ -31,7 +31,7 @@
1172 assert_equal(1, p1:get_workers("barbarians_builder"))
1173
1174 cancel_expedition_in_shipwindow(expedition_ship)
1175- sleep(20000)
1176+ sleep(50000)
1177 assert_equal(1, p1:get_workers("barbarians_builder"))
1178 check_wares_in_port_are_all_there()
1179

Subscribers

People subscribed via source and target branches

to status/vote changes: