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

Hardcoded notification sounds now configurable and optional. #15357

Merged
merged 12 commits into from Sep 24, 2018

Conversation

Projects
None yet
5 participants
@IceReaper
Copy link
Contributor

IceReaper commented Jul 18, 2018

Made all hardcoded notification sounds configurable, except the ui clicks and chatline, which are optional and do not cause a crash if not found. Also nulled all defaults so mods do never explicit need to override them with "" for proper consistency and mod friendlyness. Added these defaults to the core mods yaml files. I left out some traits which are explicit meant to just add a notification, as it makes no sense to make them optional here.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jul 18, 2018

This looks like the wrong approach to me. The game should crash if mods define bogus sounds. Isn't it possible to blank "missing" notifications out on the yaml level?

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jul 18, 2018

In my eyes thats the wrong solution.

Yamls do not expect you to write down all values, just the ones you need to set. This approach would be the opposite: You need to set all stuff you do not need, and set it to empty.

Secondly, the default values are added because they match OpenRA core mods, however, if OpenRA should be a generic RTS engine, such crashing values should get nulled and their defaults moved over into the core mods. Their current default is basicaly tailored to the westwood games, and not generic because of the crash behavior.

For KKnD i had ~ 20-30 Notifications which i just dont need at all. And i personaly dont see a valid reason why i should name all these sounds if i dont want them? I can accept the fact if you want it to crash, but then i will make this PR quite bigger and replace all the defaults with null entries and add all the entries to the mod yamls.

Just tell me the preferred way, and ill change it to whatever way you want it to work ;)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 18, 2018

IMO the proper fix here is to have the notification types default to null, then the mods that do use then should explicitly opt-in to them. See e.g. #14892.

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch from 3e13cc2 to ec36c8a Jul 22, 2018

@IceReaper IceReaper changed the title No longer crash when a mod does not have a specific notification. Hardcoded notification sounds now configurable and optional. Jul 22, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jul 22, 2018

Updated!

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch from ec36c8a to 2aff200 Jul 22, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 22, 2018

This will need an upgrade rule.

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch from 2aff200 to 92cc6b0 Jul 25, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 25, 2018

Adding the changes requested label for the update rule. https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/UpdateRules/Rules/DefineSoundDefaults.cs might be useful as a starting point.

@@ -335,7 +335,7 @@ public float VideoSeekPosition

// Returns true if played successfully
public bool PlayPredefined(SoundType soundType, Ruleset ruleset, Player p, Actor voicedActor, string type, string definition, string variant,
bool relative, WPos pos, float volumeModifier, bool attenuateVolume)
bool relative, WPos pos, float volumeModifier, bool attenuateVolume, bool optional = false)

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

These changes shouldn't be needed anymore, right?

This comment has been minimized.

@IceReaper

IceReaper Jul 25, 2018

Contributor

Sadly they are, otherwise the global ui click sound will cause crashes. Its just something which comes purely from the ui and not from any trait.

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

Can we default those to null too?

This comment has been minimized.

@IceReaper

IceReaper Jul 25, 2018

Contributor

ofcourse, but then we need a global variable for the clicksound i guess? Where to put it?

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

The ChromeMetrics class / metrics.yaml.

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch 2 times, most recently from 09c0f1e to ca0f84d Jul 26, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

Would you mind splitting the UI (clicksound) changes out into their own PR? That can then be merged only needing a compatibility note on https://github.com/OpenRA/OpenRAModSDK/wiki/Update-notes:-release-20180307-to-next-release.

The rest of the changes here will still need an update rule.

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch 3 times, most recently from 4b58c28 to eb1b761 Jul 26, 2018

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch 2 times, most recently from c05efbc to cb5e6c0 Aug 15, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Aug 15, 2018

Rebased and added UpdateRule.

@IceReaper IceReaper force-pushed the IceReaper:optional_notifications branch 2 times, most recently from a16e743 to 8c50d3e Aug 15, 2018

@Mailaender
Copy link
Member

Mailaender left a comment

Makes sense to not hard-code them.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 23, 2018

There are some issues with the update rule in here that I plan to fix. I will push over and merge once i'm happy with that.

@pchote pchote self-assigned this Sep 23, 2018

@pchote pchote force-pushed the IceReaper:optional_notifications branch from 8c50d3e to 2344001 Sep 24, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 24, 2018

Updated:

  • New update rule - the old one didn't actually work properly!
  • Fixed IngameMenuLogic to behave properly on game exit
  • Reworded some commit messages to get them below the line-break limit
@pchote
Copy link
Member

pchote left a comment

Code changes look good and I tested that the RA mod notifications still work. Trusting your process (and other reviewers) on the other mods.

@abcdefg30 abcdefg30 force-pushed the IceReaper:optional_notifications branch from 2344001 to e91e773 Sep 24, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Fixed a minor oversight. 👍 now.

@abcdefg30 abcdefg30 merged commit f342ecf into OpenRA:bleed Sep 24, 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 Sep 24, 2018

@IceReaper IceReaper deleted the IceReaper:optional_notifications branch Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment