-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add transient text notifications matching speech notifications #19872
Add transient text notifications matching speech notifications #19872
Conversation
66f360c
to
498772b
Compare
Original D2k had a few text notifications for stuff they didn't have a sound notification for, i think those could be included here too: #12209 |
498772b
to
078d4d0
Compare
{ | ||
Game.Sound.PlayNotification(world.Map.Rules, null, "Speech", info.Notification, world.RenderPlayer == null ? null : world.RenderPlayer.Faction.InternalName); | ||
TextNotificationsManager.AddTransientLine(info.TextNotification); | ||
} | ||
} | ||
|
||
void INotifyGameLoaded.GameLoaded(World world) | ||
{ | ||
if (!world.IsReplay) | ||
{ | ||
Game.Sound.PlayNotification(world.Map.Rules, null, "Speech", info.LoadedNotification, world.RenderPlayer == null ? null : world.RenderPlayer.Faction.InternalName); | ||
TextNotificationsManager.AddTransientLine(info.LoadedTextNotification); | ||
} | ||
} | ||
|
||
void INotifyGameSaved.GameSaved(World world) | ||
{ | ||
if (!world.IsReplay) | ||
{ | ||
Game.Sound.PlayNotification(world.Map.Rules, null, "Speech", info.SavedNotification, world.RenderPlayer == null ? null : world.RenderPlayer.Faction.InternalName); | ||
TextNotificationsManager.AddTransientLine(info.SavedTextNotification); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason these don't display in RA and TS 🤷
078d4d0
to
72d3afa
Compare
Update:
|
Update:
The TESTCASE commit is there only to make sure all notifications are working. The proposed notifications are implemented in the commit before that one. Reviews are welcome. |
72d3afa
to
fb041da
Compare
Width: PARENT_RIGHT | ||
Height: 20 | ||
Font: Regular | ||
Text: UI Feedback in Transients Panel | ||
Text: Show Transient Notifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the average user will understand what Transient Notifications are. I didn't anyways.
Perhaps 'Show Speech Notifications as Text'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that "transient notifications" is not quite clear but that's what they are. And there isn't a 1:1 mapping between speech and text notifications. Perhaps adding a tooltip that explains what the setting does? Or another variant for the label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a tooltip would do the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dragunoff this is still open.(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this?
I think that covers it.
About #19872 (comment): We should decide what to do for spectators in regards to notifications. That will also resolve #19662 (comment). |
fb041da
to
cc7b7ad
Compare
Update: addressed formatting nits and #19872 (comment) |
For the record: We agreed on Discord to simply not have notifications for spectators for the moment. |
Do we want to leave this checked as a default setting? |
I think it's a good default so better leave it checked. We can adjust if we get feedback from playetests. |
cc7b7ad
to
bfb640d
Compare
Update:
|
I assume this would be a followup, but what's the feasibility of exposing this to work with |
Exposing the transients tray to custom messages from lua is something that had not crossed my mind but is actually a good idea. Support for that should come as a separate method though (e.g. |
There are several speech notifications that are played directly and not via a trait. Those would need some sort of way to get transient messages. (The originals also send the current objective as transient message from time to time, but I don't think we have to necessarily redo that.) |
bfb640d
to
5cf70d3
Compare
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as promised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objection to the code changes, but see my comment below re inserting actor names and translations. I generally agree with @penev92's concern about the proliferation of trait fields, but I think that is a more general problem than just this PR and should not be solved here.
I only very briefly looked ingame - are others happy enough with the implementation for the testcase commit to be removed?
5cf70d3
to
ccf42b7
Compare
Update: rebased and addressed comments by @pchote
|
ccf42b7
to
a327182
Compare
Force pushed to remove the testcase commit. |
This PR adds support for text notifications that display along all speech notifications. This is useful for players that don't have audio or whose hearing is impaired. The notifications are displayed in the new transients panel which also hosts the UI feedback notifications. There is an option under "Display" to toggle these notifications and they are enabled by default1.
The text notifications are added as fields to traits that make use of
[NotificationReference]
23. This is a different approach to my previous attempt where the notifications were transcriptions attached to the speech definitions (see #18929). This new approach results in more code but I think it's better by design because audio and text aren't the same thing - text might not be a direct transcription of the spoken words (e.g. faction announcers) or the audio notification might not even be speech (e.g. sound effects).Screenshots
List of notifications
I have added all possible notifications to all mods in a TESTCASE commit. We should consider which texts are worthy of keeping. Some of them are backed by other visual cues (e.g. "Building.", "On hold.", "Cancelled", "Select target.", "Rally point established.") while others may be considered noise ("Unit ready.", "Unit lost.").
Here're all traits and my proposal on what notifications to include:
Common
TD
RA
D2k
TS
Linked issues
Closes #3278
Closes #6400
Closes #6354
Closes #12209
Closes #12954
Related #19724
Footnotes
I've changed the label of the UI feedback checkbox in settings to read "Show UI Feedback Notifications" (see https://github.com/OpenRA/OpenRA/pull/19677#discussion_r778010108). ↩
Text notifications for cash ticks not implemented because they are too frequent. ↩
Text notifications for beacons not implemented because I think this needs some more thought and is better left to a follow up. ↩