Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Dec 30, 2020

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')

@SamuXarick SamuXarick closed this Dec 31, 2020
@SamuXarick SamuXarick deleted the industry-sort-by-production-with-cargo-filter branch December 31, 2020 17:17
@SamuXarick SamuXarick restored the industry-sort-by-production-with-cargo-filter branch December 31, 2020 17:21
@SamuXarick SamuXarick reopened this Dec 31, 2020
@glx22
Copy link
Contributor

glx22 commented Jan 1, 2021

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

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

glx22 commented Jan 5, 2021

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
Copy link
Contributor Author

I noticed Sort by Transported also needs a similar treatment.

@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 9f5e117 to dc772d1 Compare January 5, 2021 21:30
@SamuXarick 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 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
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fairly minor

src/industry_gui.cpp Outdated Show resolved Hide resolved
Comment on lines 1492 to 1559
/* 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);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/industry_gui.cpp Show resolved Hide resolved
@SamuXarick 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
src/industry_gui.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 2c21e08 to f8beb3a Compare February 21, 2021 19:45
if (p1 > p2) Swap(p1, p2); // lower value has higher priority
CargoID filter = IndustryDirectoryWindow::produced_cargo_filter;

int p = 0, c = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from f8beb3a to 3f3cc29 Compare March 5, 2021 21:57
@SamuXarick SamuXarick requested a review from LordAro March 5, 2021 22:18
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
if (p1 > p2) Swap(p1, p2); // lower value has higher priority
CargoID filter = IndustryDirectoryWindow::produced_cargo_filter;

int p = 0, c = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/industry_gui.cpp Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 3f3cc29 to c63fe24 Compare March 10, 2021 15:36
struct CargoInfo {
CargoID cargo_id;
uint16 production;
char *suffix;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unowning, should be const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad. Fixed it.

Comment on lines 1446 to 1460
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);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1451 to 1452
if (transported != -1) produced_cargo_count++;
percentage += transported == -1 ? 0 : transported;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. it is done.

@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from c63fe24 to 96e033b Compare March 29, 2021 09:53
@SamuXarick SamuXarick requested a review from LordAro March 29, 2021 09:58
@glx22 glx22 merged commit 26f7f59 into OpenTTD:master Aug 11, 2021
@SamuXarick SamuXarick deleted the industry-sort-by-production-with-cargo-filter branch August 30, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants