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

[Benedikt Straub]

Missing wares (and workers) are indicated differently in InputQueueDisplays if the ware is on the way to the building than if it's really missing

[Notabilis (notabilis27)]

Good feature, I like it.

The structure of the code looks good, but there might be a bug in the calculations. When testing I reduced the maximum amount of requested wares for a building. It seems as if the number of darker shadow (the wares on their way?) is always the number of wares I reduced it by (e.g., building can store 10 logs, I clicked "store less" twice, now there are two darker shadows and 8 lighter ones).
Not quite sure whether the darker ones really are the requested ones, but it looks strange either way. From an UI perspective I would prefer the icon-order "wares currently stored", "wares on their way" (darker/more visible), "wares (possibly requested but) not on their way" (lighter/more transparent).

Also, I would prefer to not make the "lighter shadows" more transparent that they are in trunk. For some wares they are pretty hard to see and recognize at it is (blackwood, I think) and it becomes even more difficult with this branch.

[Benedikt Straub (nordfriese)]

The bug was that request_->get_open_count() also seems to count wares that could be brought to the building at once if suddenly needed. Fixed this now by defining in the GUI that all wares beyond the player-defined limit are shown as missing unless they´re present.

From left to right in the queue: Wares in the building (normal); wares on their way (dark); missing completely (light). I reset the transparency for missing wares to the trunk value.

Please also check wares that are already more or less monochrome (like coal, wheat, cloth), do you think the three states are distinguishable enough?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3702. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406665001.
Appveyor build 3501. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_inputwarequeue_display-3501.

[Notabilis (notabilis27)]

Thanks, it seems much more logical now. Mostly, that is, sometimes the "dark" section changes its size based on the current max-amount (i.e., 2 cloth requested -> 2 dark; 3 cloth requested -> only 1 dark). Might be correct inside the code based on how requests are calculated, guess you can't do much about that.

I think coal is fine, wheat and cloth are a bit hard to differ but in my opinion it is good enough. You can see the difference, so I think you can leave it as it is now.

An unfortunate bug I noticed: I have 1 cloth in my whole economy (based on ware statistics). With two shipyards each requesting 2 cloth, all the 2*2=4 cloth is displayed as "on their way" which just can't be the case. In the end, only 1 cloth is delivered while the rest is still reported as "on their way". I guess request_->get_num_transfers() should have better been called request_->get_num_requests() ? I haven't looked in the code, maybe there is some method which really returns the number of "wares on their way to fulfill this request". :-/

[Benedikt Straub (nordfriese)]

I´ll look into that bug… it can´t be the fault of get_num_transfers() because although I provide that function, I don´t use it for the calculations, so the problem is probably that request_->get_open_count() doesn´t do what it should…

[Benedikt Straub (nordfriese)]

I think I found it. request_->get_num_transfers() is actually the right function here, but merely passing through it´s return value isn´t enough. I made InputQueue::get_missing() more intelligent to account for capacity and existing inputs itself. I can´t reproduce the bug anymore now.
I also added an assert to check that the calculations at least make sense.

[GunChleoc (gunchleoc)]

1 small comment for performance

[Benedikt Straub (nordfriese)]

Implemented your suggestion. WaresQueue returns only a variable, but WorkersQueue calls std::vector::size(), so this is more efficient.

[Notabilis (notabilis27)]

I can't reproduce my bug any longer either, so as far as I am concerned this can go in. Thanks!

review: Approve

[GunChleoc (gunchleoc)]

@bunnybot merge

=== modified file 'src/economy/input_queue.cc'
--- src/economy/input_queue.cc	2018-04-07 16:59:00 +0000
+++ src/economy/input_queue.cc	2018-07-22 15:42:30 +0000
@@ -120,6 +120,14 @@
 	update();
 }
 
+uint32_t InputQueue::get_missing() const {
+	const auto filled = get_filled();
+	if (filled >= max_fill_ || request_ == nullptr || !request_->is_open()) {
+		return 0;
+	}
+	return max_fill_ - filled - std::min(max_fill_, request_->get_num_transfers());
+}
+
 constexpr uint16_t kCurrentPacketVersion = 3;
 
 void InputQueue::read(FileRead& fr, Game& game, MapObjectLoader& mol) {

=== modified file 'src/economy/input_queue.h'
--- src/economy/input_queue.h	2018-04-07 16:59:00 +0000
+++ src/economy/input_queue.h	2018-07-22 15:42:30 +0000
@@ -102,6 +102,14 @@
 	virtual Quantity get_filled() const = 0;
 
 	/**
+	 * The amount of missing wares or workers which have been requested
+	 * but are not yet being transported to this building.
+	 * This will never be larger than (get_max_fill()-get_filled()).
+	 * @return The amount at this moment.
+	 */
+	uint32_t get_missing() const;
+
+	/**
 	 * Clear the queue appropriately.
 	 * Implementing classes should call update() at the end to remove the request.
 	 */

=== modified file 'src/wui/inputqueuedisplay.cc'
--- src/wui/inputqueuedisplay.cc	2018-04-27 06:11:05 +0000
+++ src/wui/inputqueuedisplay.cc	2018-07-22 15:42:30 +0000
@@ -129,7 +129,12 @@
 	cache_max_fill_ = queue_->get_max_fill();
 
 	uint32_t nr_inputs_to_draw = std::min(queue_->get_filled(), cache_size_);
-	uint32_t nr_empty_to_draw = cache_size_ - nr_inputs_to_draw;
+	uint32_t nr_missing_to_draw = std::min(queue_->get_missing(), cache_max_fill_) + cache_size_ - cache_max_fill_;
+	if (nr_inputs_to_draw > cache_max_fill_) {
+		nr_missing_to_draw -= nr_inputs_to_draw - cache_max_fill_;
+	}
+	uint32_t nr_coming_to_draw = cache_size_ - nr_inputs_to_draw - nr_missing_to_draw;
+	assert(nr_inputs_to_draw + nr_missing_to_draw + nr_coming_to_draw == cache_size_);
 
 	Vector2i point = Vector2i::zero();
 	point.x = Border + (show_only_ ? 0 : CellWidth + CellSpacing);
@@ -139,10 +144,15 @@
 		dst.blitrect(Vector2i(point.x, point.y), icon_, Recti(0, 0, icon_->width(), icon_->height()),
 		             BlendMode::UseAlpha);
 	}
-	for (; nr_empty_to_draw; --nr_empty_to_draw, point.x += CellWidth + CellSpacing) {
-		dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_,
-		                              Recti(0, 0, icon_->width(), icon_->height()),
-		                              RGBAColor(166, 166, 166, 127));
+	for (; nr_coming_to_draw; --nr_coming_to_draw, point.x += CellWidth + CellSpacing) {
+		dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_,
+		                              Recti(0, 0, icon_->width(), icon_->height()),
+		                              RGBAColor(127, 127, 127, 191));
+	}
+	for (; nr_missing_to_draw; --nr_missing_to_draw, point.x += CellWidth + CellSpacing) {
+		dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_,
+		                              Recti(0, 0, icon_->width(), icon_->height()),
+		                              RGBAColor(191, 191, 191, 127));
 	}
 
 	if (!show_only_) {