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

Throttles for notification sounds so rapid user actions do not produce annoying sounds #15360

Merged
merged 1 commit into from Dec 23, 2018

Conversation

Projects
None yet
8 participants
@drogoganor
Copy link
Contributor

drogoganor commented Jul 19, 2018

One of the first things I noticed in OpenRA that deviated from the originals was no throttling or selective omitting of EVA notification sounds when the user rapidly queues/cancels items in the build palette. It's probably also worth a mention that when I recently played a LAN game of OpenRA, my friends would consistently giggle at all the "Build-build-build-building" and "Can-can-can-cancel" sounds going on. This is totally killing me in Dark Reign because the sounds are "Unit generation in progress" and "Construction under way" which are really long sounds!

So I have a proposed way of throttling these sounds which uses NotificationSoundThrottle traits attached to the World object like so:

NotificationSoundThrottle@building:
	Delay: 800
	Sounds: Building
NotificationSoundThrottle@cancelled:
	Delay: 500
	Sounds: Cancelled
NotificationSoundThrottle@training:
	Delay: 800
	Sounds: Training
NotificationSoundThrottle@unitready:
	Delay: 500
	Sounds: UnitReady

The delay is in milliseconds and the Sounds are a list of Notifications key names (see notifications.yaml if unfamiliar)

I am accomplishing the throttle in Sound.PlayPredefined(...) like so:

// Check whether this sound is in throttle rules and suppress if so
if (p != null)
{
	var soundThrottles = p.World.WorldActor.TraitsImplementing<NotificationSoundThrottle>().Where(x => x.IsSoundPartOfPool(p, definition));
	foreach (var t in soundThrottles)
	{
		if (t.IsSoundThrottled(p, definition))
			return true; // Count as success
		t.UpdateLastSoundPlayed();
		break;
	}
}

I have a couple of questions about this approach:

  1. Is this trait named OK? I thought about mentioning that it omits sounds so maybe something like "NotificationSoundOmitThrottle" instead?
  2. Should the traits be attached to the World object?
  3. Looking up the throttles using p.World.WorldActor.TraitsImplementing<NotificationSoundThrottle>() . Should this be done in another way?
  4. Performance. Is this going to be too much of a performance penalty?

Anyway, it feels a lot nicer, but I might suggest the Building throttle delay be reduced a little because users can queue several different buildings quickly without it being "spammy" and it would be good to get a"Building" sound for every click.

Related to #6908 .

Cheers for your time

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 19, 2018

It would be best if the rate limit could be defined on the notification itself, e.g.:

Notifications:
	[...]
	Building: bldging1
		RateLimit: 800
	[...]

Trait queries incur a performance hit, so we want to avoid doing these each time a notification is (potentially) triggered.

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Jul 19, 2018

Yeah I originally wanted to do that, but wasn't sure how to go about modifying SoundInfo, which stores the key names and sounds in Dictionary<string, string[]> Voices. It then loads this with FieldLoader.Load(this, y);

Is there an example somewhere of the kind of syntax you specified? i.e. a custom list of names and values, with traits attached? (I'm not really sure of the terminology).

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Jul 19, 2018

Well, I have altered things so the yaml is on the notifications like so:

Speech:
	Notifications:
		BaseAttack: ATACK
	...
	NotificationRateLimits:
		Building: 600
		Training: 800
		UnitReady: 900
		Cancelled: 500

And the SoundInfo.GetNext() simply returns null if the sound is currently in the limit window, which doesn't need a lookup for each sound played.

It's nicer but I'm not sure how to implement the code for the yaml syntax you've suggested in the SoundInfo class.

@Mailaender
Copy link
Member

Mailaender left a comment

This works like promised.

Show resolved Hide resolved OpenRA.Game/Sound/Sound.cs Outdated

@drogoganor drogoganor force-pushed the drogoganor:notification-throttle branch 3 times, most recently from d2966fe to e9c561d Oct 18, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Oct 18, 2018

Updated to use syntax as suggested:

Notifications:
 	[...]
	Building: bldging1
 		RateLimit: 800
	[...]
Show resolved Hide resolved OpenRA.Game/GameRules/SoundInfo.cs
Show resolved Hide resolved OpenRA.Game/GameRules/SoundInfo.cs Outdated
Show resolved Hide resolved OpenRA.Game/GameRules/SoundInfo.cs Outdated

@pchote pchote added this to the Next Release milestone Nov 21, 2018

Show resolved Hide resolved OpenRA.Game/GameRules/SoundInfo.cs Outdated
Show resolved Hide resolved OpenRA.Game/GameRules/SoundInfo.cs Outdated
@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Nov 23, 2018

Related: #3449, #4690

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 25, 2018

I don't think we can justify delaying a christmas playtest over this, so bumping to Next + 1.

@pchote pchote modified the milestones: Next Release, Next + 1 Nov 25, 2018

@drogoganor drogoganor force-pushed the drogoganor:notification-throttle branch from e9c561d to 1df3c98 Nov 28, 2018

@drogoganor drogoganor force-pushed the drogoganor:notification-throttle branch from 1df3c98 to ab95ed7 Dec 2, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Dec 2, 2018

Rebased.

@pchote

pchote approved these changes Dec 23, 2018

@pchote pchote merged commit bbc83c1 into OpenRA:bleed Dec 23, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

drogoganor commented Dec 25, 2018

Thank you!

@drogoganor drogoganor deleted the drogoganor:notification-throttle branch Dec 25, 2018

@Punsho

This comment has been minimized.

Copy link

Punsho commented Jan 7, 2019

There is a lot of negative feedback about this feature in discord, now the queueing and canceling of units feels highly unresponsive making people feel like they are missclicking. My suggestion would be either having a shorter audio notification or a more silent one. Dropping notifications is simply not the way to go

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jan 7, 2019

Standing up against the entire feature just because the shipped configuration ain't good enough is unfair though. Tweak the notification yamls and propose better values.

@Punsho

This comment has been minimized.

Copy link

Punsho commented Jan 7, 2019

I don't understand this reaction, I'm just saying that the current implementation is flawed and it should be fixed, I'm just providing feedback and am not demanding anything

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