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

Remove remaining internal .aud sound defaults from Mods.Common #14892

Merged
merged 4 commits into from Mar 9, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 9, 2018

They were fine when RA and TD were the only mods, but now that there are several non-C&C 3rd-party mods in development and possibly more in the future, it would be more modder-friendly as well as more consistent with the majority of sound properties in Mods.Common to get rid of the remaining internal .aud defaults.

I left Mods.Cnc untouched for now, since MadTank and the two chrono traits are pretty RA-specific.

reaperrr added some commits Sep 10, 2017

Remove legacy .aud sound defaults from Common traits
While C&C-specific sound defaults might be acceptable for C&C-specific traits like MadTank and Chronoshiftable, for common, generic traits like Building they no longer are.
{
if (node.Key == "ParaDrop")
{
var soundNodePD = node.Value.Nodes.FirstOrDefault(n => n.Key == "ChuteSound");

This comment has been minimized.

@PeterAntal

PeterAntal Mar 9, 2018

Contributor

Code style question - Is there a reason there's not a helper method to deal with repetition of conditionally adding?

It seems like the following pattern could express your intent for all these cases...
AddOnMissingNode(nodes, key, value), used like:
AddOnMissingNode(node.Value.Nodes, "ChuteSound","chute1.aud")

This comment has been minimized.

@reaperrr

reaperrr Mar 9, 2018

Author Contributor

Code style question - Is there a reason there's not a helper method to deal with repetition of conditionally adding?

Apart from that nobody has bothered writing one yet, not really.

@pchote has started writing a (currently about half-finished) complete replacement for the current upgrade rule system though, so at this point it's questionable whether such a helper method would have any tangible benefit for the (hopefully short) remaining lifetime of the current system.

This comment has been minimized.

@pchote

pchote Mar 21, 2018

Member

For reference, this is how it looks on the new system: https://github.com/pchote/OpenRA/blob/4f779e91c2712709776dbb9cd22662defb59f5a0/OpenRA.Mods.Common/UpdateRules/Rules/DefineSoundDefaults.cs

However, note that this works slightly differently: it prints the locations that might want to be changed, rather than changing everything and then relying on the person running the command implicitly knowing that they need to undo most of them.

@abcdefg30 abcdefg30 merged commit bd09773 into OpenRA:bleed Mar 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 9, 2018

@reaperrr reaperrr deleted the reaperrr:remove-common-aud-defs branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.