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.

Sign up for GitHub

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

Jump to bottom

Trait Modify Factions #955

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+64 −0
Open

Trait Modify Factions #955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
300460a
Trait Modify Factions
VMSolidus Sep 22, 2024
d7a8420
Apply suggestions from code review
VMSolidus Sep 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions Content.Server/Traits/TraitSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Content.Shared.Psionics;
using Content.Server.Language;
using Content.Shared.Mood;
using Content.Server.NPC.Systems;

namespace Content.Server.Traits;

Expand All @@ -28,6 +29,7 @@ public sealed class TraitSystem : EntitySystem
[Dependency] private readonly PsionicAbilitiesSystem _psionicAbilities = default!;
[Dependency] private readonly IComponentFactory _componentFactory = default!;
[Dependency] private readonly LanguageSystem _languageSystem = default!;
[Dependency] private readonly NpcFactionSystem _factionSystem = default!;

public override void Initialize()
{
Expand Down Expand Up @@ -71,6 +73,8 @@ public void AddTrait(EntityUid uid, TraitPrototype traitPrototype)
AddTraitLanguage(uid, traitPrototype);
RemoveTraitLanguage(uid, traitPrototype);
AddTraitMoodlets(uid, traitPrototype);
RemoveTraitFactions(uid, traitPrototype);
AddTraitFactions(uid, traitPrototype);
}

/// <summary>
Expand Down Expand Up @@ -225,4 +229,28 @@ public void AddTraitMoodlets(EntityUid uid, TraitPrototype traitPrototype)
if (_prototype.TryIndex(moodProto, out var moodlet))
RaiseLocalEvent(uid, new MoodEffectEvent(moodlet.ID));
}

/// <summary>
/// If a trait includes any faction removals, this removes the faction from the receiving entity.
/// </summary>
public void RemoveTraitFactions(EntityUid uid, TraitPrototype traitPrototype)
{
if (traitPrototype.RemoveFactions is null)
return;

Comment on lines +238 to +240
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
if (traitPrototype.RemoveFactions is null)
return;

Copy link
Member Author

Choose a reason for hiding this comment

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

Death for the love of god no. We've had this conversation before.

Copy link
Member

@DEATHB4DEFEAT DEATHB4DEFEAT Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/6455672
Null lists are pointlessly extra to deal with and probably worse performing, though negligible either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also still wrong, that's from 13 years ago. That predates Dotnet 8 by 12 YEARS.
Null List is the preferred method in Dotnet 8, even according to Microsoft's documentation for Dotnet 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

This is painfully fucking outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Death this link you've posted is relevant to dotnet 4.

Copy link
Member

Choose a reason for hiding this comment

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

I know

Copy link
Member

Choose a reason for hiding this comment

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

I found another that better explains what I mean: https://stackoverflow.com/a/1970010

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Death this is even more outdated, and irrelevant to what we're doing here. Death the entire reason why the people in StackOverflow(FROM A LITERAL DECADE AGO) are saying not to use Nullable lists is because in older versions of C#, the compiler had no way of finding NullReferences before Runtime, so errors when coding with nullable variables WAS (past tense) difficult to debug.. Nullable List is preferred over Empty List in Dotnet 8 because it's:

  • several times faster to guard clause the null conditional vs. entering a foreach with an empty list.
  • The dotnet 8 compiler(And IDEs using it) can identify null reference conditions at compile time, you can see this in Intellisense straight up telling you whether variables are potentially null. The compiler will error out if you fail to check a null condition, then tell you the exact line where you failed to check it.
  • Build And Test Debug will Test-Fail if you foreach an empty list. Which is why even if you wanted me to do this system over empty lists, I can't, because instead of checking if the list is null, I now have to check if the list is empty. Which is slower and less performant than checking if the list is null.

Here it's being used in the entire TraitSystem because it's the fastest possible way to go through a very wide assortment of lists. It's faster to check which lists are null, than it is to foreach loop each and every list in the prototype to check if they aren't empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the stackoverflow answers explicitly mention that non-nullable empty lists have worse performance, but are better for maintainability, usability, and readability. That has not changed in a decade.

foreach (var faction in traitPrototype.RemoveFactions)
_factionSystem.RemoveFaction(uid, faction);
}

/// <summary>
/// If a trait includes any factions to add, this adds the factions to the receiving entity.
/// </summary>
public void AddTraitFactions(EntityUid uid, TraitPrototype traitPrototype)
{
if (traitPrototype.AddFactions is null)
return;

VMSolidus marked this conversation as resolved.
Show resolved Hide resolved
foreach (var faction in traitPrototype.AddFactions)
_factionSystem.AddFaction(uid, faction);
}
}
18 changes: 18 additions & 0 deletions Content.Shared/Traits/Prototypes/TraitPrototype.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,22 @@ public sealed partial class TraitPrototype : IPrototype
/// </summary>
[DataField]
public List<ProtoId<MoodEffectPrototype>>? MoodEffects { get; private set; } = default!;

/// <summary>
/// The list of all Factions that this trait removes.
/// </summary>
/// <remarks>
/// I can't actually Validate these because the proto lives in Shared.
VMSolidus marked this conversation as resolved.
Show resolved Hide resolved
/// </remarks>
[DataField]
public List<string>? RemoveFactions { get; private set; } = default!;
VMSolidus marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// The list of all Factions that this trait adds.
/// </summary>
/// <remarks>
/// I can't actually Validate these because the proto lives in Shared.
/// </remarks>
[DataField]
public List<string>? AddFactions { get; private set; } = default!;
VMSolidus marked this conversation as resolved.
Show resolved Hide resolved
}
4 changes: 4 additions & 0 deletions Resources/Locale/en-US/traits/traits.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,7 @@ trait-description-Spinarette =
trait-name-AddictionNicotine = Nicotine Addiction
trait-description-AddictionNicotine =
You have an addiction to Nicotine, and will require frequent smoke breaks to keep your mood in check.

trait-name-AnimalFriend = Animal Friend
trait-description-AnimalFriend =
You have a way with animals. You will never be attacked by animals, unless you attack them first.
7 changes: 7 additions & 0 deletions Resources/Prototypes/Traits/skills.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,3 +282,10 @@
- !type:CharacterSpeciesRequirement
species:
- IPC

- type: trait
id: AnimalFriend
category: Mental
points: -4
addFactions:
- AnimalFriend
7 changes: 7 additions & 0 deletions Resources/Prototypes/ai_factions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@

- type: npcFaction
id: SimpleHostile
friendly:
- AnimalFriend
hostile:
- NanoTrasen
- Syndicate
Expand Down Expand Up @@ -86,6 +88,8 @@

- type: npcFaction
id: Pibble
friendly:
- AnimalFriend
hostile:
- Cat
- Birb
Expand All @@ -99,3 +103,6 @@

- type: npcFaction
id: Birb

- type: npcFaction
id: AnimalFriend
Loading