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 Dec 31, 2020
@SamuXarick SamuXarick restored the industry-sort-by-production-with-cargo-filter branch Dec 31, 2020
@SamuXarick SamuXarick reopened this Dec 31, 2020
@glx22
Copy link
Contributor

@glx22 glx22 commented Jan 1, 2021

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

Loading

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

@glx22 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.

Loading

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 5, 2021

I noticed Sort by Transported also needs a similar treatment.

Loading

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

@LordAro LordAro left a comment

Fairly minor

Loading

src/industry_gui.cpp Outdated Show resolved Hide resolved
Loading
/* 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

@LordAro LordAro Feb 13, 2021

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

Loading

Copy link
Contributor Author

@SamuXarick SamuXarick Feb 21, 2021

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

Loading

Copy link
Member

@LordAro LordAro Feb 21, 2021

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)

Loading

src/industry_gui.cpp Outdated Show resolved Hide resolved
Loading
src/industry_gui.cpp Show resolved Hide resolved
Loading
@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch 3 times, most recently from 0780d49 to 2c21e08 Feb 18, 2021
src/industry_gui.cpp Outdated Show resolved Hide resolved
Loading
@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from 2c21e08 to f8beb3a Feb 21, 2021
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

@stormcone stormcone Feb 21, 2021

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? :)

Loading

Copy link
Member

@LordAro LordAro Mar 10, 2021

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

Loading

@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from f8beb3a to 3f3cc29 Mar 5, 2021
@SamuXarick SamuXarick requested a review from LordAro Mar 5, 2021
src/industry_gui.cpp Outdated Show resolved Hide resolved
Loading
src/industry_gui.cpp Outdated Show resolved Hide resolved
Loading
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

@LordAro LordAro Mar 10, 2021

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

Loading

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

@LordAro LordAro Mar 28, 2021

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

Loading

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 29, 2021

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.

Loading

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

@LordAro LordAro Mar 28, 2021

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

Loading

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 29, 2021

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.

Loading

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

@LordAro LordAro Mar 28, 2021

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

Loading

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 29, 2021

Choose a reason for hiding this comment

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

Thanks. it is done.

Loading

@SamuXarick SamuXarick force-pushed the industry-sort-by-production-with-cargo-filter branch from c63fe24 to 96e033b Mar 29, 2021
@SamuXarick SamuXarick requested a review from LordAro Mar 29, 2021
LordAro
LordAro approved these changes May 1, 2021
@glx22 glx22 merged commit 26f7f59 into OpenTTD:master Aug 11, 2021
12 checks passed
Loading
@SamuXarick SamuXarick deleted the industry-sort-by-production-with-cargo-filter branch Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants