Topic: Transportation priorities
|
abtools
Joined: 2010-01-08, 15:31
Posts: 31  Pry about Widelands Location: Munich |
Posted at: 2010-11-21, 11:25
Hello SirVer, if this approach really results in priority wares to get transported first (and it works correctly, so that the priority is calculated in a way probably also a user would define it in a manual GUI) then I absolutely can live with this approach, too. I just didn't thought that it is possible to implement this automatic priority calculation in such an easy way, but in this case it's the best solution, of course. Best regards Andreas 
Top
Quote
|
|
ixprefect
Joined: 2009-02-27, 13:28
Posts: 367  Tribe Member |
Posted at: 2010-11-21, 13:27
abtools: Cutting and pasting patches from pastebin won't work because of whitespace changes, but there should be a download option. As for the patch, as I wrote elsewhere the abs() is not correct. In fact after some discussion, it seems that actually the multiplication with modifier has to go outside the std::max. Also, please adapt the comment style and put the #define together with the others. Ultimately, the DEBUG #ifdefs should also be removed. Edit: Oh, I forgot, it seems like the WAIT_BONUS is applied in the wrong place. I have to admit that I'm not totally happy with the entire thing, since it just adds more kludges onto a bunch of existing kludges. It doesn't add any understanding of how the system works as a whole. Edited: 2010-11-21, 13:28
Top
Quote
|
|
nsh
Joined: 2010-11-08, 19:30
Posts: 27 Pry about Widelands |
Posted at: 2010-11-21, 14:13
1) Well, we agreed at least on the existence of bug in get_priority(). abs() is wrong there, however, it actually made the impact of bug less. But didn't correct it. 2) WAIT_BONUS is actually Ok. Both tasks are actually separate and therefore should be treated separately.
Top
Quote
|
|
nsh
Joined: 2010-11-08, 19:30
Posts: 27 Pry about Widelands |
Posted at: 2010-11-21, 16:10
Playing around with it i have found that the core problem is in get_priority(). It is designed to be called only once - when the ware being allocated. Therefore, it has to consider negative and positive time shifts. abs() incorrect here, agree. But, after the ware is allocated and already being transferred, both time shifts will be negative - indeed, if item is already allocated, it means that it's request time is in past relative to the in-game time. Here abs() usage may be excessive, but anyway correct. Current realization of get_priority() obviously can not handle it correctly (I doubt, that it gives correct results even for initial call), that's why abs() corrects this thing. But abs() is not correct in the first place for initial call to the get_priority(), when the ware is being evaluated for allocation. Looks like, this function should return priority value depending on the state of the request. In initial call for any ware it should return the economical priority to determine ware allocation, but after allocation - transportation priority. And they differ only in how one sign of time shifts is considered Have to think, how to implement it correctly. UPD - no, the sign of time shift for constructionsite is always negative, and for ware may be either positive or negative. Positive values should be ignored anyway (delay the ware allocation for future requests). So, get_priority() should work correctly no matter, when it was called. Current implementation does not possess this behavior => contains the bug. Moreover, the single code instance that calls get_priority expects negative priority values to delay that ware allocation, while actually it returns only positive values>=100. Only a small part of code, avoiding delivery to buildings, that will not be idling without this ware, is active right now. Therefore, ware allocation (not only transportation) is actually FIFO! Because no matter what priority would be estimated for the request, it will succeed due to being always positive. Corresponding part of economy.cc """ int32_t const priority = req.get_priority (cost); if (priority < 0) continue; """ As you see, it has no sense, it always pass through. I think that by moving priority estimation out of max(), as ixprefect suggested, this may be actually fixed, because priorities in this case may become negative. Edited: 2010-11-21, 18:56
Top
Quote
|
|
nsh
Joined: 2010-11-08, 19:30
Posts: 27 Pry about Widelands |
Posted at: 2010-11-21, 19:31
1) In fact it means that get_priority() does not have any sense for ware allocation, it's value is safely ignored later. It may be even not called at all, nothing will change 2) That's why, the fix with abs() that I proposed, nonetheless is actually correct, because it calculates transportation priorities correctly. And it doesn't change anything in ware allocation - see 1)
Top
Quote
|
Venatrix
Topic Opener
Joined: 2010-10-05, 19:31
Posts: 449 Tribe Member Location: Germany |
Posted at: 2010-11-21, 20:35
Is it right, if I understand, that the player won't be able to change the transportation priority of a single ware for the whole transportation system but only of a ware destination by changing the priority in the building menu? Hmm... it's definitely another approach than I had in mind. Maybe one can't disturb the own economy so much in this way. As I'm one of those, who play only the trunk-rev, I have to wait, if this patch will go there, before testing it myself. I rely on the judgement of the developers Edited: 2010-11-21, 20:43
Two is the oddest prime.
Top
Quote
|
|
ixprefect
Joined: 2009-02-27, 13:28
Posts: 367 Tribe Member |
Posted at: 2010-11-22, 15:44
Okay, might I suggest simply adding a function Transfer::get_priority() that should indicate some form of priority for the transfer, from scratch? After all, to decide which ware should be taken from a flag, it is really the Transfer priority (a concept that simply doesn't exist yet) that matters. Recall that there are Transfers that do not have an associated Request. Then we can deal with priority questions of satsifying Requests separately. From what I've understood so far, this is probably the cleanest way to approach it. Transfer priority and Request priority are conceptually different things - one being used to choose among wares waiting at a flag, the other being used to control the matching of Request and Supply. So it makes sense to have separate priority computing functions as well. Then you can make a clean suggestion for how the priority of Transfers should be determined, and at some later point we can fix the mess that is Request/Supply matching.
Top
Quote
|
|
nsh
Joined: 2010-11-08, 19:30
Posts: 27 Pry about Widelands |
Posted at: 2010-11-22, 18:39
Ok, thanks, I completely understand your point of view and support it. Anyway, I have a feeling that now I understand how request->get_priority() was supposed to work and what is wrong there. I created a corresponding bug report, hopefully it will be helpful.
Top
Quote
|
|
nsh
Joined: 2010-11-08, 19:30
Posts: 27 Pry about Widelands |
Posted at: 2010-11-26, 21:30
Hi! Please, see the https://bugs.launchpad.net/widelands/+bug/681934. I submitted it as a bug there because don't know any better place where the patch could be easily attached and reused. All explanations on the logic is also given there. I am quite satisfied with result, as there is no magic, pure logic
Top
Quote
|
|
abtools
Joined: 2010-01-08, 15:31
Posts: 31 Pry about Widelands Location: Munich |
Posted at: 2010-11-27, 11:08
Hello nsh, just wanted to ask if you got some news regarding these issues already. Your detailed info in bug https://bugs.launchpad.net/widelands/+bug/680164 looked very promising. Did you have some time to test it further already? I'm really looking forward to a better implementation of the request and transportation priority. Best regards Andreas
Top
Quote
|
