Skip to content
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

Language Refactor 3 #937

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Mnemotechnician
Copy link
Contributor

@Mnemotechnician Mnemotechnician commented Sep 20, 2024

Description

This significantly improves the quality of the language system by fixing the mistakes I've made almost a year ago while developing it.

Mainly, this throws away the old half-broken way of networking in favor of the component state system provided by RT. Language speaker comp is now shared with SendOnlyToOwner = true, and its state is handled manually.

In addition to that, this brings the following changes:

  • UniversalLanguageSpeaker and LanguageKnowledge are now server-side
  • DetermineLanguagesEvent and LanguagesUpdateEvent are now shared (so that future systems can be built in shared, if needed)
  • Everything now uses the ProtoId type instead of raw strings (god, I hated those so much)
  • The server-side language system now accepts Entity<T?> arguments instead of EntityUid + T
  • UniversalLanguageSpeaker is now based on DetermineEntityLanguagesEvent and gets an Enabled field, which allows to turn it off. This may have some use in the future.
  • Some minor cleanup

Changelog

No cl

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: UI Changes any XAML files labels Sep 20, 2024
@FoxxoTrystan FoxxoTrystan requested review from FoxxoTrystan, a team, VMSolidus, DEATHB4DEFEAT and Pspritechologist and removed request for a team September 20, 2024 05:40
@github-actions github-actions bot added the Status: Needs Review Someone please review this label Sep 20, 2024

/// <summary>
/// List of languages this entity can speak at the current moment.
/// </summary>
public List<string> SpokenLanguages = [];
[DataField]
public List<ProtoId<LanguagePrototype>> SpokenLanguages = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public List<ProtoId<LanguagePrototype>> SpokenLanguages = [];
public List<ProtoId<LanguagePrototype>>? SpokenLanguages = default!;

We really should be doing these as Nullable lists and not a default empty list. Anywhere you logically check "If empty return", you can instead check "if null return". This is actually overwhelmingly preferred now according to Microsoft's own documentation, ever since Dotnet 8 added the ability for IDE's to check for null references before compiling. The given reason is that it offers a substantial performance improvement, while still using the same general logic structure.

Copy link
Contributor Author

@Mnemotechnician Mnemotechnician Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the benefit, but I see how it would be highly inconvenient for every caller to check those fields for null, when in reality they would never be null. The performance difference should be very negligible here, although if it concerns you a lot, I simply can change the initializer to default!, without changing the field type.

@Mnemotechnician
Copy link
Contributor Author

Everything works as far as I could test, so marking as ready for review.

@Mnemotechnician Mnemotechnician marked this pull request as ready for review September 22, 2024 14:27
@VMSolidus
Copy link
Member

And that's why I merge master before testing PRs now. Excellent. This PR doesn't work. :)

@Mnemotechnician
Copy link
Contributor Author

merge is unrelated, I broke the linter somehow.

@Mnemotechnician
Copy link
Contributor Author

The linter was unhappy I had an empty string as the default value for LSC.CurrentLanguage. Well, I revoked its right to check that field.

Copy link
Member

@VMSolidus VMSolidus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Can't approve this until you add back in the functionality for removing UniversalLanguageSpeakerComponent also removing the Universal Language. Here I have a Cataloguer that has been mindbroken(which removes the UniversalLanguageSpeakerComponent), but even after the component is removed, the Cataloguer retained their ability to speak Universal.

@Mnemotechnician
Copy link
Contributor Author

Mnemotechnician commented Sep 22, 2024

Can't approve this until you add back in the functionality for removing UniversalLanguageSpeakerComponent also removing the Universal Language. Here I have a Cataloguer that has been mindbroken(which removes the UniversalLanguageSpeakerComponent), but even after the component is removed, the Cataloguer retained their ability to speak Universal.

Mmmm, this just means the xenoglossy system is broken as it does not call UpdateEntityLanguages when adding/removing language-related comps.

I'll make it get called automatically on the comp removal event... later, when I get time.

@FoxxoTrystan FoxxoTrystan added Priority: 2-High Needs to be resolved as soon as possible Size: 2-Large For large issues/PRs labels Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Changes: UI Changes any XAML files Priority: 2-High Needs to be resolved as soon as possible Size: 2-Large For large issues/PRs Status: Needs Review Someone please review this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants