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

@abmyii abmyii commented Sep 27, 2019

Fixes #7703.

@nielsmh
Copy link
Contributor

nielsmh 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
Copy link
Contributor Author

abmyii commented Sep 28, 2019

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

@abmyii
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor

J0anJosep commented Sep 28, 2019 via email

@andythenorth
Copy link
Contributor

andythenorth commented Sep 29, 2019

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

@abmyii
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor

andythenorth 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor

andythenorth 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii commented Oct 1, 2019

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

@abmyii
Copy link
Contributor Author

abmyii commented Oct 6, 2019

Any follow-up?

@glx22
Copy link
Contributor

glx22 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
Copy link
Contributor Author

abmyii commented Oct 6, 2019

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

@abmyii
Copy link
Contributor Author

abmyii commented Oct 6, 2019

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

@abmyii
Copy link
Contributor Author

abmyii commented Oct 6, 2019

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

@glx22
Copy link
Contributor

glx22 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor

glx22 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
Copy link
Contributor Author

abmyii commented Oct 6, 2019

Yes, please!

src/industry_cmd.cpp Outdated Show resolved Hide resolved
@abmyii
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii commented Oct 6, 2019

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

@glx22
Copy link
Contributor

glx22 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
Copy link
Contributor Author

abmyii commented Oct 7, 2019

Done, thanks for the tip.

@planetmaker
Copy link
Contributor

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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii 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
Copy link
Contributor

planetmaker 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
Copy link
Contributor Author

abmyii commented Oct 7, 2019

Great, thanks again!

@abmyii
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii commented Oct 8, 2019

Anything else required for this PR?

src/industry_cmd.cpp Outdated Show resolved Hide resolved
@abmyii
Copy link
Contributor Author

abmyii commented Oct 9, 2019

Thanks for the approval! What else is remaining?

@LordAro
Copy link
Member

LordAro commented Oct 9, 2019

Patience :p

@abmyii
Copy link
Contributor Author

abmyii 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
Copy link
Contributor Author

abmyii commented Oct 11, 2019

Have I been patient long enough (wink)?

@nielsmh nielsmh merged commit ac21118 into OpenTTD:master Oct 12, 2019
@abmyii
Copy link
Contributor Author

abmyii commented Oct 12, 2019

Great, thanks a lot!

@abmyii abmyii deleted the fix_inactive_sounds branch October 12, 2019 09:46
douiwby pushed a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
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.

Inactive industries make inappropriate/unlogical sounds
9 participants