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

Prevent sounds being produced by inactive industries #7752

Merged
merged 4 commits into from Oct 12, 2019

Conversation

@abmyii
Copy link
Contributor

commented Sep 27, 2019

Fixes #7703.

@nielsmh

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

Hmm, all the regression tests are failing, and the reason must be because the consumption of random numbers has changed. Notice there is a Chance16 call in the branch of the if no longer taken when production is zero. Since that gets skipped, the RNG for the rest of the game gets out of sync with the "expected". This isn't wrong, but it makes the output for the purpose of regression testing different.

Additionally, the commit checker is rejecting your commit because the commit message does not start with a keyword. (See this about commit messages.) Update the commit message with git commit --amend and force-push, Github handles that just fine.
Suggestion: Fix #7703: Prevent sounds being produced by inactive industries

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

In that case should I let Chance16 be called then check last_month_production before playing sounds?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

Sorry about the merge commit - I cloned the main repository (this) rather than my fork, so the merge took place when I pulled before amending. Still trying to find my way around with git!

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

I tried make test to test my changes but I kept on getting this error despite everything I tried:
+Error: Failed to find a graphics set. Please acquire a graphics set for OpenTTD. See section 4.1 of README.md.

src/industry_cmd.cpp Outdated Show resolved Hide resolved
@J0anJosep

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

@andythenorth

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

Is it defined what 'inactive' is for a newgrf industry? :)

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

@andythenorth

Is it defined what 'inactive' is for a newgrf industry? :)

I assume so, but I haven't tested it yet. I take it you have already done so?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

I tried with FIRS Replacement Set 3, however it doesn't seem to have any industry sounds - I didn't hear anything anyway.

@andythenorth

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

For newgrf industry, there is simply no way to know in advance what behaviour is defined 'inactive'.

I don't think industry sounds are particularly important for newgrf, but I am just one opinion.

But if this PR is merged, it would be worth noting in the newgrf docs (nfo and nml) that sound effects aren't purely random, and may never trigger for newgrf industries. Potentially a newgrf author could spend a long time investigating why sound effects don't work / no longer work as expected.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

True, I never thought of that - being new to the inner workings of OpenTTD. Where/how would that be documented? Also, surely the definition of inactivity chosen (last month's production) should be fairly ubiquitously applicable?
Thanks for all of the insight you have provided!

@andythenorth

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Relevant newgrf docs are:

They don't promise more than sound is played 'sometimes', and just need a note explaining the conditions under which sounds won't be played. That wouldn't need doing unless the PR is accepted.

I would not mind never hearing the sawmill noise ever again, but maybe that's just me 😃

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Should that be part of this PR? Also, is there a repo for the wiki or should the editions be done on the wiki itself (I expect this is the correct method)?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

I would not mind never hearing the sawmill noise ever again, but maybe that's just me smiley

Haha! Funnily enough I've always played without sound since I never could be bothered to take the extra steps of downloading and enabling the sounds.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Any updates on what else needs to be done to this PR?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Any follow-up?

@glx22

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

I still fail to understand why the commit checker was successful, but you'll need to use git rebase -i HEAD~6 to squash all your commits into one.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Thanks for that. I've applied it and pushed. Hopefully it works this time!

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Changes made to check all of the industry's outputs.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

I can't seem to fix this tab indentation (of all things) despite my efforts. Any ideas?

@glx22

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

You need to fix it in the commit introducing the wrong indentation, as the commit checker checks the diffs individually

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Oh my, may I ask what the easiest way to do that would be? Sorry for my basic questions and thanks for all your help!

@glx22

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

The easiest way is to rebase, but it seems your previous rebase somehow failed. I can force push a rebase on your branch if you want.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Yes, please!

@glx22 glx22 force-pushed the abmyii:fix_inactive_sounds branch from fe7d9b0 to e738181 Oct 6, 2019
src/industry_cmd.cpp Outdated Show resolved Hide resolved
@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Thank you for resolving that problem @glx22! I believe that now the PR should be complete (hopefully!).

src/industry_cmd.cpp Outdated Show resolved Hide resolved
@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Sorry for tiring you with such trivial problems throughout @glx22. You've been a great help!

@glx22

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

Arg, github commits don't follow our style. But it's easily fixable with git commit --amend and a forced push. Do you want to try it ?

@abmyii abmyii force-pushed the abmyii:fix_inactive_sounds branch from b4eb561 to b50aeff Oct 7, 2019
@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Done, thanks for the tip.

@planetmaker

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

I've one question regarding black-hole industries (that is industries which only receive cargo but don't produce anything like power plants): I don't see how they will be able to make sounds with this patch. Do I miss something?
If not, please also add as an or-condition a check for received cargo :)

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

It doesn't apply to them, since I can hear their sounds despite no cargo being transported to them. I never bothered checking why, but I expect the i->last_month_production is never 0 for them.
Thanks for your input!

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Haha, you made me curious and I couldn't help but check. As was mentioned in the issue, the code for the power station sound is here:

case GFX_POWERPLANT_SPARKS:
if (Chance16(1, 3)) {
if (_settings_client.sound.ambient) SndPlayTileFx(SND_0C_ELECTRIC_SPARK, tile);
AddAnimatedTile(tile);
}
break;

Whereas the code for the production sounds is in the function ProduceIndustryGoods:

OpenTTD/src/industry_cmd.cpp

Lines 1120 to 1139 in b50aeff

static void ProduceIndustryGoods(Industry *i)
{
const IndustrySpec *indsp = GetIndustrySpec(i->type);
/* play a sound? */
if ((i->counter & 0x3F) == 0) {
uint32 r;
uint num;
if (Chance16R(1, 14, r) && (num = indsp->number_of_sounds) != 0 && _settings_client.sound.ambient) {
for (size_t j = 0; j < lengthof(i->last_month_production); j++) {
if (i->last_month_production[j] > 0) {
/* Play sound since last month had production */
SndPlayTileFx(
(SoundFx)(indsp->random_sounds[((r >> 16) * num) >> 16]),
i->location.tile);
break;
}
}
}
}

This does raise another question though - are there any NewGRF/extension packs with black-hole industries that play their sounds through the ProduceIndustryGoods function? I doubt it but I am not very familiar with how they work. If it was the case, what would be checked to identify one?

@planetmaker

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

ah, right. Looking at the broader context (that is beyond the patch), that function is only called for producing industries. :) And the accepting default industries are handled via the tile loop separately :) Cheers!

EDIT: NewGRFs can also make use of the tile loop. or run callback on receiving cargo at which time they can play a sound.

But you actually pointed out the place an additional patch might go: making the blackhole industries kinda inactive unless they actually receive something to eat :D (But if so: that's for a separate PR)

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Great, thanks again!

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@planetmaker With regards to the point you added, that'd have to be done by someone with a better idea of how the engine works since I expect that a new attribute will have to be added, unless there is a more obvious way that I don't know?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Anything else required for this PR?

src/industry_cmd.cpp Outdated Show resolved Hide resolved
@LordAro
LordAro approved these changes Oct 9, 2019
@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Thanks for the approval! What else is remaining?

@LordAro

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Patience :p

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Haha, and I thought it would only require changing one or two lines and be done in a day!
I'm still not used to the PRing and I expect this isn't an abnormally long time to wait, but my preconceived expectations will obviously need adjusting. Pardon my impatience!

@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

Have I been patient long enough (wink)?

@nielsmh nielsmh merged commit ac21118 into OpenTTD:master Oct 12, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20191009.1 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
@abmyii

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

Great, thanks a lot!

@abmyii abmyii deleted the abmyii:fix_inactive_sounds branch Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.