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
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1126,9 +1126,18 @@ static void ProduceIndustryGoods(Industry *i)
uint32 r;
uint num;
if (Chance16R(1, 14, r) && (num = indsp->number_of_sounds) != 0 && _settings_client.sound.ambient) {
This conversation was marked as resolved by abmyii

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 9, 2019

Member

not strictly your issue, but i'd want to see num (and it's declaration) moved inside the if statement

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 9, 2019

Author Contributor

Should I do that as part of my PR?

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 9, 2019

Author Contributor

Also, why should it be moved inside? Is it because it is bad practice to declare in the statement?

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 9, 2019

Member

Bad practice to declare variables in a scope above where they're used. Also, assignment in if statements is unpleasant in all but the simplest of cases.

Not sure why an extra variable is needed at all, indsp->random_sounds[((r >> 16) * indsp->number_of_sounds) >> 16] would work just fine, though I can't say I understand what it's actually doing with all the bitshifts. Probably some remnant of the original TTD ASM.

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 9, 2019

Author Contributor

Is indsp->number_of_sounds a constant? I assumed it was defined by the industry, and it's set here:

indsp->number_of_sounds = buf->ReadByte();

It may just be the same for them all though.

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 9, 2019

Member

regardless, it's not modified in this function

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 9, 2019

Author Contributor

Push a commit removing it. Could you please verify it is what you wanted?

SndPlayTileFx(
(SoundFx)(indsp->random_sounds[((r >> 16) * num) >> 16]),
i->location.tile);
bool playsound = false;
for (size_t j = 0; j < lengthof(i->last_month_production); j++) {
if (i->last_month_production[j] > 0) {
playsound = true;
break;
}
}

if (playsound)
SndPlayTileFx(
(SoundFx)(indsp->random_sounds[((r >> 16) * num) >> 16]),
i->location.tile);
This conversation was marked as resolved by abmyii
Comment on lines +1137 to +1140

This comment has been minimized.

Copy link
@PeterN

PeterN Oct 6, 2019

Member

Missing { } for the if-block, but the SndPlayTileFx() call could just be before the break; above.

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 6, 2019

Author Contributor

Your observation is correct - thank you! As for the lack of braces, they aren't required if the if statement only has one line inside. I don't think that is a style requirement for OpenTTD since I have seen it being done elsewhere in the code.

This comment has been minimized.

Copy link
@Eddi-z

Eddi-z Oct 6, 2019

Contributor

According to our codestyle, the braces are required whenever the if spans more than one line.

}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.