-
Notifications
You must be signed in to change notification settings - Fork 146
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:
|
|
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
…Station#34 (Simple-Station#51) <!-- Please read these guidelines before opening your PR: https://docs.spacestation14.io/en/getting-started/pr-guideline --> <!-- The text between the arrows are comments - they will not be visible on your PR. --> ## About the PR <!-- What did you change in this PR? --> Ported tweaks ## Why / Balance <!-- Why was it changed? Link any discussions or issues here. Please discuss how this would affect game balance. --> Tweaks. ## Technical details <!-- If this is a code change, summarize at high level how your new code works. This makes it easier to review. --> Yaml changes. ## Media <!-- PRs which make ingame changes (adding clothing, items, new features, etc) are required to have media attached that showcase the changes. Small fixes/refactors are exempt. Any media may be used in SS14 progress reports, with clear credit given. If you're unsure whether your PR will require media, ask a maintainer. --> ## Requirements <!-- Due to influx of PR's we require to ensure that PR's are following the correct guidelines. Please take a moment to read these if its your first time. Check the boxes below to confirm that you have in fact seen these (put an X in the brackets, like [X]): --> - [X] I have read and I am following the [Pull Request Guidelines](https://docs.spacestation14.com/en/general-development/codebase-info/pull-request-guidelines.html). I understand that not doing so may get my pr closed at maintainer’s discretion - [X] I have added screenshots/videos to this PR showcasing its changes ingame, **or** this PR does not require an ingame showcase ## Breaking changes <!-- List any breaking changes, including namespace, public class/method/field changes, prototype renames; and provide instructions for fixing them. This will be pasted in #codebase-changes. --> None, I hope. **Changelog** <!-- Make players aware of new features and changes that could affect how they play the game by adding a Changelog entry. Please read the Changelog guidelines located at: https://docs.spacestation14.io/en/getting-started/pr-guideline#changelog --> <!-- Make sure to take this Changelog template out of the comment block in order for it to show up. Changelog must have a 🆑 symbol, so the bot recognizes the changes and adds them to the game's changelog.--> 🆑 TwoDucksOnnaPlane & TheGreenChkrta - tweak: Added back damage tweaks
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