Fix #8316: Make sort industries by production and transported with a cargo filter possible #8468

SamuXarick · 2020-12-30T11:38:55Z

Motivation / Problem

#8316
When sorting by production with a cargo filter on industries that produce more than one cargo, it would sort by the sum of all cargoes produced, and not by the chosen cargo.
EDIT:
Sorting by transported only accounts for the 2 first produced cargoes of an industry. If an industry produces more cargoes, the transported percentage of those cargoes would not be evaluated.
Even in the case of industries producing just 2 cargoes, the transported % was not displayed orderly in the string of a single industry, nor would the percentage be sorted correctly when compared with other industries.

Description

The sort by production problem is solved by making the sorter function able to know which cargo filter is applied, then sort by either the sum of cargoes when any cargo is selected, or only by the cargo selected.

The sort by transported problem is a bit tricky. The string displaying the various details is now sorted by cargo transported, instead of cargo production. Then it also knows which cargo filter is applied. If all cargo types is selected, then it sorts the list by the average of percentages of cargoes transported. If a cargo type is selected, then it sorts the list by that cargo percentage.

Limitations

I think the sort by production problem is solved in all scenarios. There was also a cargo filter that would sort by none. I simply made it not go through the whole cargoes that the industries produce, only to return zero.

The sort by transported problem has industries that don't produce any cargo at all. I thought of grouping them all together, similar to the original code, but in the reverse position, instead of a "101%", it's a "-1%". I made it this way due to code being based on the sum of percentages, and when iterating over invalid cargoes of industries that produce at least one cargo, it returns a +"0%" to the sum. All in the name of making some sense in the sorted list.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

The bug fix is important enough to be backported? (label: 'backport requested')
This PR affects the save game format? (label 'savegame upgrade')
This PR affects the GS/AI API? (label 'needs review: Script API')
- ai_changelog.hpp, gs_changelog.hpp need updating.
- The compatibility wrappers (compat_*.nut) need updating.
This PR affects the NewGRF API? (label 'needs review: NewGRF')
- PR must be added to API tracker

glx22 · 2021-01-01T00:08:35Z

Not fan of the global variables, there should be a cleaner way.

glx22 · 2021-01-05T17:52:27Z

In glx22@e42ab5a I tried something a bit cleaner.
Ideally we should pass the filter to Sort(), but that implies touching all sorters in the source.

SamuXarick · 2021-01-05T18:04:16Z

I noticed Sort by Transported also needs a similar treatment.

LordAro

Fairly minor

src/industry_gui.cpp

LordAro · 2021-02-13T15:52:17Z

src/industry_gui.cpp

+				/* Sort by descending production, then descending transported */
+				std::sort(cargos.begin(), cargos.end(), [](const CargoInfo a, const CargoInfo b) {
+					if (std::get<1>(a) != std::get<1>(b)) return std::get<1>(a) > std::get<1>(b);
+					return std::get<3>(a) > std::get<3>(b);
+				});
+				break;
+
+			case IDW_SORT_BY_TRANSPORTED:
+				/* Sort by descending transported, then descending production */
+				std::sort(cargos.begin(), cargos.end(), [](const CargoInfo a, const CargoInfo b) {
+					if (std::get<3>(a) != std::get<3>(b)) return std::get<3>(a) > std::get<3>(b);
+					return std::get<1>(a) > std::get<1>(b);
+				});


Probably best to turn CargoInfo into a proper struct, so these rather ugly sort functions get better names

I'm not sure how to do this

Instead of

typedef std::tuple<CargoID, uint16, const char*, uint> CargoInfo;

use

struct CargoInfo { Foo foo; Bar bar; Baz baz; };

Then instead of std::get<3>(a) it becomes a.baz etc
(You can define the struct within the function, that's fine)

src/industry_gui.cpp

stormcone · 2021-02-21T20:05:21Z

src/industry_gui.cpp

-		if (p1 > p2) Swap(p1, p2); // lower value has higher priority
+		CargoID filter = IndustryDirectoryWindow::produced_cargo_filter;
+
+		int p = 0, c = 0;


I would like to see some comments about what is going on here in that function. And what p, c, & t is for? :)

Indeed. We're not limited on space, use your words :)

src/industry_gui.cpp

LordAro · 2021-03-09T20:15:49Z

src/industry_gui.cpp

-		if (p1 > p2) Swap(p1, p2); // lower value has higher priority
+		CargoID filter = IndustryDirectoryWindow::produced_cargo_filter;
+
+		int p = 0, c = 0;


Indeed. We're not limited on space, use your words :)

src/industry_gui.cpp

LordAro · 2021-03-28T13:19:49Z

src/industry_gui.cpp

+		struct CargoInfo {
+			CargoID cargo_id;
+			uint16 production;
+			char *suffix;


This is unowning, should be const

Oh, my bad. Fixed it.

LordAro · 2021-03-28T13:30:18Z

src/industry_gui.cpp

+		for (uint id = 0; id < lengthof(i->produced_cargo); id++) {
+			if (filter == CF_NONE) {
+				return percentage;
+			} else if (filter == CF_ANY) {
+				int transported = GetCargoTransportedPercentsIfValid(i, id);
+				if (transported != -1) produced_cargo_count++;
+				percentage += transported == -1 ? 0 : transported;
+				if (produced_cargo_count == 0 && id == lengthof(i->produced_cargo) - 1 && percentage == 0) {
+					return transported;
+				}
+			} else if (filter == i->produced_cargo[id]) {
+				return GetCargoTransportedPercentsIfValid(i, id);
+			}
+		}


This for loop (and the one in the sorter) is weird - it still enters the loop even if filter == CF_NONE which is unconnected to the loop

filter == CF_NONE is now outside the two for loops.

LordAro · 2021-03-28T13:31:35Z

src/industry_gui.cpp

+				if (transported != -1) produced_cargo_count++;
+				percentage += transported == -1 ? 0 : transported;


Should consolidate the transported checks

Suggested change

if (transported != -1) produced_cargo_count++;

percentage += transported == -1 ? 0 : transported;

if (transported != -1) {

produced_cargo_count++;

percentage += transported;

}

Thanks. it is done.

…with a cargo filter possible

SamuXarick closed this Dec 31, 2020

SamuXarick deleted the industry-sort-by-production-with-cargo-filter branch December 31, 2020 17:17

SamuXarick restored the industry-sort-by-production-with-cargo-filter branch December 31, 2020 17:21

SamuXarick reopened this Dec 31, 2020

glx22 added a commit to glx22/OpenTTD that referenced this pull request Jan 5, 2021

Alternate version for OpenTTD#8468

e42ab5a

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 9f5e117 to dc772d1 Compare January 5, 2021 21:30

SamuXarick changed the title ~~Fix #8316: Make sort industries by production with a cargo filter possible~~ Fix #8316: Make sort industries by production and transported with a cargo filter possible Jan 5, 2021

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch 2 times, most recently from a1cc2e1 to c8af7ac Compare January 10, 2021 18:24

LordAro requested changes Feb 13, 2021

View reviewed changes

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch 3 times, most recently from 0780d49 to 2c21e08 Compare February 18, 2021 15:52

LordAro reviewed Feb 21, 2021

View reviewed changes

src/industry_gui.cpp Outdated Show resolved Hide resolved

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 2c21e08 to f8beb3a Compare February 21, 2021 19:45

stormcone reviewed Feb 21, 2021

View reviewed changes

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from f8beb3a to 3f3cc29 Compare March 5, 2021 21:57

SamuXarick requested a review from LordAro March 5, 2021 22:18

LordAro reviewed Mar 10, 2021

View reviewed changes

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 3f3cc29 to c63fe24 Compare March 10, 2021 15:36

LordAro reviewed Mar 28, 2021

View reviewed changes

Fix OpenTTD#8316: Make sort industries by production and transported …

96e033b

…with a cargo filter possible

SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from c63fe24 to 96e033b Compare March 29, 2021 09:53

SamuXarick requested a review from LordAro March 29, 2021 09:58

LordAro approved these changes May 1, 2021

View reviewed changes

glx22 merged commit 26f7f59 into OpenTTD:master Aug 11, 2021

SamuXarick deleted the industry-sort-by-production-with-cargo-filter branch August 30, 2021 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #8316: Make sort industries by production and transported with a cargo filter possible #8468

Fix #8316: Make sort industries by production and transported with a cargo filter possible #8468

SamuXarick commented Dec 30, 2020 •

edited

Loading

glx22 commented Jan 1, 2021

glx22 commented Jan 5, 2021

SamuXarick commented Jan 5, 2021

LordAro left a comment

LordAro Feb 13, 2021

SamuXarick Feb 21, 2021

LordAro Feb 21, 2021

stormcone Feb 21, 2021

LordAro Mar 9, 2021

LordAro Mar 9, 2021

LordAro Mar 28, 2021

SamuXarick Mar 29, 2021

LordAro Mar 28, 2021

SamuXarick Mar 29, 2021

LordAro Mar 28, 2021

SamuXarick Mar 29, 2021

		if (transported != -1) produced_cargo_count++;
		percentage += transported == -1 ? 0 : transported;

Fix #8316: Make sort industries by production and transported with a cargo filter possible #8468

Fix #8316: Make sort industries by production and transported with a cargo filter possible #8468

Conversation

SamuXarick commented Dec 30, 2020 • edited Loading

Motivation / Problem

Description

Limitations

Checklist for review

glx22 commented Jan 1, 2021

glx22 commented Jan 5, 2021

SamuXarick commented Jan 5, 2021

LordAro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamuXarick commented Dec 30, 2020 •

edited

Loading