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

Industry directory cargo filtering #7843

Merged
merged 3 commits into from Jan 5, 2020

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.

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

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

LordAro
LordAro previously approved these changes Dec 23, 2019
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
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.

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

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

src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants