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

@stormcone stormcone commented Nov 23, 2019

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

@James103 James103 commented Nov 23, 2019

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

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

@stormcone stormcone commented Nov 27, 2019

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 stormcone:industry-directory-filter branch from 1197087 to 79c1d62 Nov 28, 2019
@stormcone stormcone force-pushed the stormcone:industry-directory-filter branch from 79c1d62 to ed01ac4 Dec 16, 2019
@stormcone stormcone requested a review from LordAro Dec 16, 2019
Copy link
Member

@LordAro LordAro left a comment

Fine by me

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

@stormcone stormcone commented Dec 23, 2019

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 Dec 23, 2019
@James103
Copy link
Contributor

@James103 James103 commented Dec 23, 2019

@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

@stormcone stormcone commented Dec 23, 2019

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

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]);

This comment has been minimized.

@LordAro

LordAro Dec 23, 2019
Member

instead of this cast, could IndustryTemporarilyRefusesCargo not have its signature changed? Would seem to me to be more "correct"

This comment has been minimized.

@stormcone

stormcone Dec 24, 2019
Author Contributor

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.

This comment has been minimized.

@LordAro

LordAro Dec 24, 2019
Member

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

This comment has been minimized.

@stormcone

stormcone Dec 24, 2019
Author Contributor

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
stormcone added 3 commits Nov 23, 2019
…ut if it temporarily does not accept the cargo selected by the acceptance cargo filter.
@stormcone stormcone force-pushed the stormcone:industry-directory-filter branch from 5b024c9 to c38e91c Dec 24, 2019
@stormcone stormcone requested a review from LordAro Dec 24, 2019
@James103
Copy link
Contributor

@James103 James103 commented Dec 24, 2019

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 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
8 checks passed
8 checks passed
OpenTTD CI Build #20191224.6 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@stormcone stormcone deleted the stormcone:industry-directory-filter branch Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.