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.

Sign up for GitHub

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

Jump to bottom

Industry directory cargo filtering #7843

Merged
merged 3 commits into from
Jan 5, 2020
Merged

Industry directory cargo filtering #7843

merged 3 commits into from
Jan 5, 2020
+232 −13

Conversation

stormcone
Copy link
Contributor

This first commit comes from @KeldorKatarn's patch pack:
KeldorKatarn/OpenTTD_PatchPack@b213294
I've made some minor changes to fit the current code base.

The second one is from me.

Here is a screenshot about how it looks like in-game:
industry_cargo_filter

@James103
Copy link
Contributor

The following is a practical situation that many players will come across with this patch: If an industry temporarily stops accepting or producing a cargo, and the industry window is set to only show industries that accept or produce that cargo, the industry will _____?
a) Disappear from the list
b) Stay on the list where it is
c) Become greyed out
d) Move to the bottom of the list and be grayed out

For me, I like choice (c) best as it shows that the industry isn't producing/accepting that cargo, but has a capability of producing/accepting that cargo again. (d) could be a good option, but may break sorting.

LordAro
LordAro requested changes Nov 27, 2019
View reviewed changes
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.

Nothing major, I like it

As James suggested, there might be a missing InvalidateWindowData call in the various places where Industry production/creation/deletion happens. Maybe not, but worth a look

@glx22 glx22 mentioned this pull request Nov 27, 2019
Merged
@stormcone
Copy link
Contributor Author

As James suggested, there might be a missing InvalidateWindowData call in the various places where Industry production/creation/deletion happens. Maybe not, but worth a look

In the current form (b) happens, because industries' accepted cargo property does not change when a cargo is becoming temporary not accepted. There is a separate method to find this out from a callback result.

Currently the window is refreshed only every hundredth tick, so if (c) or (d) would be implemented it could cause some delays.
I am planning to implement (c), so I will look at it.

@stormcone stormcone force-pushed the industry-directory-filter branch from 1197087 to 79c1d62 Compare November 28, 2019 00:18
@stormcone stormcone force-pushed the industry-directory-filter branch from 79c1d62 to ed01ac4 Compare December 16, 2019 20:20
@stormcone stormcone requested a review from LordAro December 16, 2019 20:35
LordAro
LordAro previously approved these changes Dec 23, 2019
View reviewed changes
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.

Fine by me

@stormcone stormcone force-pushed the industry-directory-filter branch from ed01ac4 to 5b024c9 Compare December 23, 2019 20:11
@stormcone
Copy link
Contributor Author

I added one more commit.

I tried to find out where should redraw the window if the cargo acceptance changes for an industry, but I did not figure out. So the window is still only refreshed at every 100th tick.

@stormcone stormcone requested a review from LordAro December 23, 2019 20:20
@James103
Copy link
Contributor

@stormcone If possible, could you please change the the industry directory refresh rate to every in-game day (every 74th tick)? That would align more with OpenTTD's calendar.

@stormcone
Copy link
Contributor Author

I would not like to change it. It is inherited through various windows. There is a timer which refreshes the windows at every 100th tick. So I think it's better if it stays as it is.

To change this behavior for every window should be done in a different PR .

LordAro
LordAro requested changes Dec 23, 2019
View reviewed changes
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.

Can I be really picky and ask that the "patch from foobar patchpack" be removed from the commit message? I think that belongs here in the PR, not the commit message

src/industry_gui.cpp
DrawString(r.left + WD_FRAMERECT_LEFT, r.right - WD_FRAMERECT_RIGHT, y, this->GetIndustryString(this->industries[i]));
tc = TC_FROMSTRING;
if (acf_cid != CF_ANY && acf_cid != CF_NONE) {
Industry *ind = const_cast<Industry *>(this->industries[i]);
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 this cast, could IndustryTemporarilyRefusesCargo not have its signature changed? Would seem to me to be more "correct"

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 think to change the signature of IndustryTemporarilyRefusesCargo then the GetIndustryCallback should be changed, and maybe other functions too. So I would not like to change it in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be happy with it in this PR as a separate commit, but am not fussed

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 tried to change it, but did not succesed. I always get error when trying to modify the IndustriesScopeResolver.

@stormcone stormcone force-pushed the industry-directory-filter branch from 5b024c9 to c38e91c Compare December 24, 2019 20:17
@stormcone stormcone requested a review from LordAro December 24, 2019 20:28
@James103
Copy link
Contributor

I notice you approved this PR and #7867, but merged #7867 first. Why did you do that? Does that mean I will have to wait until next nightly (and that the industry directory cargo filtering will not make it into OpenTTD 1.10.0-beta2)

@LordAro
Copy link
Member

LordAro commented Dec 24, 2019

Hmm, guess I forgot. My bad. Yeah, you will have to wait until next nightly/next release for this, assuming it gets merged soon

@LordAro LordAro merged commit 596fb5d into OpenTTD:master Jan 5, 2020
@stormcone stormcone deleted the industry-directory-filter branch January 5, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LordAro LordAro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@stormcone @James103 @LordAro