-
Notifications
You must be signed in to change notification settings - Fork 122
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
Languages #43
Languages #43
Conversation
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.
Whoooo boy, that's a big one. Lots of thoughts after the fact...
First off, I think it would be suitable to move the collection of language information (spoken, understood, required, requiresall, the stuff you were defining more than once) and move it into a serializable struct or class object that can be plonked into any instance that requires defining languages in YAML. This allows for the definitions to be reused, and means you can easily extend the functionality. You don't end up with the awkward situation where implementation X supports one feature, but implementation Y doesn't.
Finally, this whole thing needs to be cvar'd out the butt. I'd recommend a few cvars by default, one for the if languages are enabled at all, one for if animals can understand each other (personal preference, you'd probably want an 'animalistic' tag in the animals language), one for if animals can be translated at at all, and perhaps one for the fallback language ID? These all need to react to the cvars being updated mid game.
I know this seems like a huge amount to implement, but really it's just amazing practice for encapsulating your code and ensuring that vital functionality isn't strewn about multiple functions and systems. You shouldn't need to check the values of these cvars more than a couple times, everything should kinda 'funnel down' into your main functions.
Resources/keybinds.yml
Outdated
@@ -183,6 +183,9 @@ binds: | |||
- function: OpenCharacterMenu | |||
type: State | |||
key: C | |||
- function: OpenLanguageMenu | |||
type: State | |||
key: M |
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.
'M' personally feels like an odd choice to me. I'd align that more with maps, or other such stuff. As a random offer, I'd probably go with 'L'.
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.
key: M | |
key: L |
{ | ||
"name": "icon" | ||
}, | ||
{ | ||
"name": "translator" | ||
} |
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.
These should probably have better names than 'icon' and 'translator,' especially considering 'icon' isn't wholly accurate as it appears to be the base sprite and translator appears to only be the unshaded portion.
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.
These should probably be changed to inline lists, just to make the file a little more digestible.
Also this is super cool, I was hoping replacements would be per-language! :P
if (Transform(args.Target).ParentUid is { Valid: true } holder && EntityManager.HasComponent<LanguageSpeakerComponent>(holder)) | ||
{ | ||
// This translator is held by a language speaker and thus has an intrinsic counterpart bound to it. Make sure it's up-to-date. | ||
var intrinsic = EntityManager.EnsureComponent<HoldsTranslatorComponent>(holder); |
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.
EnsureComp
UpdatedLanguages(holder); | ||
} | ||
|
||
private void OnTranslatorToggle(EntityUid translator, HandheldTranslatorComponent component, ActivateInWorldEvent args) |
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.
Make this function call a generic, public ToggleTranslator
function to allow for interoperability
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.
private void OnTranslatorToggle(EntityUid translator, HandheldTranslatorComponent component, ActivateInWorldEvent args) | |
private void OnTranslatorToggle(EntityUid translator, HandheldTranslatorComponent component, | |
ActivateInWorldEvent args) |
intrinsic.Issuer = comp; | ||
} | ||
|
||
private static void AddIfNotExists(List<string> list, string item) |
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 in an IDE right now, but if this is not used more than I'd say remove it :P
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.
Sadly no, it's used above in two places and I hate it
@@ -45,6 +48,7 @@ namespace Content.Server.Chat.Systems; | |||
/// </summary> | |||
public sealed partial class ChatSystem : SharedChatSystem | |||
{ | |||
public const float DefaultObfuscationFactor = 0.2f; |
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.
This should absolutely not live where it is right now
{ | ||
//If listener is too far and has no line of sight, they can't identify the whisperer's identity | ||
var result = ObfuscateMessageReadability(finalMessage); | ||
var wrappedResult = Loc.GetString("chat-manager-entity-whisper-unknown-wrap-message", |
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.
Please try not to include 'magic strings' in your code like this. Any strings being accessed in code should generally be defined as consts at the top of the script to make debugging for others not a nightmare.
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.
It's not even our code, just a part of reorganized upstream code- 😭
How would you recommend to do it?
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.
Eh, honestly leave it in that case. Refactors can happen later. The only point here then would be to try and reorganize less upstream code without refactoring it.
For reference, the correct way to hardcode a value is to not hardcode a value, but to leave it as a DataField in a Component somewhere. The second best way to hardcode a value is to do it as a 'private const string = ;` at the very top of the file, to make these values clear and collected.
By the way, totally forgot to mention in the review- |
- CanilunztTranslatorImplanter | ||
- SolCommonTranslatorImplanter | ||
- RootSpeakTranslatorImplanter | ||
#- AnimalTranslator |
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.
uncomment this
var builder = new StringBuilder(); | ||
if (language.ObfuscateSyllables) | ||
{ | ||
ObfuscateSyllables(builder, message, language); | ||
} | ||
else | ||
{ | ||
ObfuscatePhrases(builder, message, language); | ||
} |
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 had a terrific idea. This should take a generic 'LanguageObfuscator' Class with children for each method of obfuscation. Instead of a binary boolean here, it would simply take the Obfuscator from the Language Prototype and execute the 'Obfuscate' function on it, using whatever is returned. This allows languages to implement custom obfuscation logic, for instance binary could be obfuscated based on actually being converted to binary (a stupid but funny suggestion), or Shadowkin could use whatever absurd system Death made.
The replacement portion of the YAML would be moved to the default obfuscator, since not every one would them.
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.
This is a good idea!
But that could be made in the future and not planned for the base code.
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.
Would nice to have things localised when this is finished as a QOL thing.
Co-authored-by: Pspritechologist <[email protected]>
I went through most of the requested changes and implemented them. But the more time I spend on this, the more I feel my sanity depleting, so I could not handle all, ask @FoxxoTrystan for the rest (mainly yml in the end of the list of changes) or something- Some comments on some things in the reviews above:
|
|
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've discovered through testing that this apparently breaks Telepathy, which lives in the same code as Server Chatsystem. Players can HEAR telepathy when spoken by the Oracle, but cannot SAY telepathy. Telepathy works as a 4th method of chat, like Say, and Whisper, but wasn't updated like those two. Telepathy needs to be updated to use languages, and such that it must work again.
List of currently known issues:
|
Will require testing now Co-authored-by: DEATHB4DEFEAT <[email protected]> Signed-off-by: Mnemotechnican <[email protected]>
…100+ suggestions in one PR
Nice to aee the assistance tho i think the "Animals Langauges/Implant code" should not be done on this list and later when the "Base" of languages are done so its avoid keeping this on limbo. For animals im gona gives langauges a bunch of new properties like how many spaces, ect... |
Mark review comments as resolved when you resolve them please. |
sadly I cannot do that, because this PR is owned by trystan |
Steal their account 🙂 |
How dare you! |
Co-authored-by: DEATHB4DEFEAT <[email protected]> Signed-off-by: FoxxoTrystan <[email protected]>
Outated/Updated Suggestions has been commented. |
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.
As far as I'm concerned this is sufficiently "Feature Complete" and free of major bugs that we can justify merging it in this exact state, and have any future updates to it be a separate PR.
ready to merge, just need a brave soul to dismiss reviews |
Update edge.yml
Resolves #37
Description
This PR adds languages. Every entity who can speak now speaks a specific language (or Universal, for entities that are not supposed to speak, which is understood by everyone). Other entities who do not understand this language will see gibberish (it's possible to learn how certain induvidual words are spelled. But the spelling changes between rounds). This means that certain creatures, like xenos, cats, vulps, can communicate within their species in their own languages. Similarly, it means that xenos, cats and other things cannot understand GalacticCommon speakers without a translator or cognization.
An entity may be able to speak multiple languages, or understand a language but be unable to speak it.
Thi PR was orignally made for Frontier but is now being ported and will be maintain here.
Orignal PR: new-frontiers-14/frontier-station-14#671
This PR was made orignally by Mnemotechnician and FoxxoTrystan.
TODO
Media
Changelog
🆑 FoxxoTrystan / Mnemotechnician