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

Trait Modify Factions #955

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

Conversation

VMSolidus
Copy link
Member

@VMSolidus VMSolidus commented Sep 22, 2024

Description

Made by request from Nuclear14, while also adding a trait that was present both in Parkstation, as well as something N14 would like to have. This PR extends the TraitSystem so that it can now also add or remove Factions from a character. It also adds in the Animal Friend trait from Parkstation/Fallout: New Vegas.

Changelog

🆑

  • add: The Animal Friend trait has been added to the game. Characters with this trait are not attacked by animals.

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: YML Changes any yml files labels Sep 22, 2024
Resources/Locale/en-US/traits/traits.ftl Outdated Show resolved Hide resolved
Comment on lines +238 to +240
if (traitPrototype.RemoveFactions is null)
return;

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

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.

Content.Server/Traits/TraitSystem.cs Show resolved Hide resolved
Resources/Prototypes/Traits/skills.yml Outdated Show resolved Hide resolved
Co-authored-by: DEATHB4DEFEAT <[email protected]>
Signed-off-by: VMSolidus <[email protected]>
@github-actions github-actions bot added the Status: Needs Review Someone please review this label Sep 23, 2024
@FoxxoTrystan FoxxoTrystan added Size: 4-Small For small issues/PRs Priority: 3-Medium Needs to be resolved at some point and removed Status: Needs Review Someone please review this 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: Localization Changes any ftl files Changes: YML Changes any yml files Priority: 3-Medium Needs to be resolved at some point Size: 4-Small For small issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants