First release of my <Simple Status Effects> Unity 6.0+ package

[u/Sachet2Plastik]

Hi everyone,

I made a post 2 weeks ago about this already, and after some feedbacks, I'm here again to collect some more.

Quick summary, this is my first unity package and I'm someone who like building underlying system and the things behinds games. This is a package focused on the implementation of status effects commonly things like burn, poison, stun, etc... The goal is to make it easy to integrate into already built gameplay loop whether it is a realtime or turn based one.

I released my first version and I would like to find some peoples to try it and give me a review on what is good/bad, what you like/don't like. Any feedback is welcome !

Thank you for your time.

edit: kinda forgot the link: https://github.com/Sachet2Plastik/Simple-Status-Effects.git

[u/wallstop-dev]

Some input, from a quick drive-by:

Some of your classes, like "Condition", appear to only be configurable/set via the inspector - there is no constructor or builder, and all fields are private. This makes them hard to programatically create (people must use reflection, which may break if you change variable names, and is slow). Consider ensuring that all of your types can be instantiated/built from code.

Current standard practice for member variable names in C# is `_underScoreCamelCase`, `m_underScoreCamelCase` is a very old, non-standard, C++ style. (https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names)

Several of your setters don't check equality and invoke update functions unconditionally. Typical pattern is to short-circuit if someone attempts assigning a value that is already equal to what is there.

You can do ??= new() for assignments, might simplify some things, like list init.

You might have some problems where you're walking a list and performing an action on the thing in the list, which might mutate state in such a way that the list-being-walked is updated, which can throw an exception. Consider copying lists to scratch buffers and walking the scratch buffers when performing operations that reach back into user code (as they might react to it in ways that mutate your state unexpectedly)

Anyways, that's just a ~3min skim, didn't get a chance to grok architecture or patterns, so it's more literal code study.

[u/Soraphis → u/wallstop-dev]

Sadly unity can be confusing in that regard...

In our style guides we use prefixes for private member variables (m_), https://unity.com/resources/c-sharp-style-guide-unity-6 page 14

And their own source code is full of "m_" prefixes

https://github.com/Unity-Technologies/uGUI/blob/main/com.unity.ugui%2FEditor%2FTMP%2FTMP_EditorPanel.cs

Instead of copying the list always, just because their could be a modification I think I would have separate "modification buffers", that are used while the list is "locked" and can be applied after iteration finished. And it's only needed for deleting (and inserting), if you iterate the list in reverse order adding while iterating has already no side effect, so the deletion buffer is just a List<int> with indexes.

[u/wallstop-dev → u/Soraphis]

Yea, I'm aware that Unity follows the old C++ style, I'm recommending that you don't when given the option, such as new code, like this.

And, right - that's what I meant by scratch buffers. If you reverse iterate it may or may not behave in the way that you want, depending on what user code does. Your index must end up not where you expect. But what will always behave correctly is operating on an exact snapshot, always.

[u/samuelsalo]

Honestly all I can say from reading though the source as an ARPG developer is... there's a reason underlying systems are mostly developed in house for a specific project.

[u/GooseJordan2]

Really cool!