Can I have a closer conversation with

[deep bronze]

Sure, but if you don't mind, let's discuss it further here so that others could learn from your case. If that's not OK, feel free to DM me

Sure, but if you don't mind, let's discuss it further here so that others could learn from your case. If that's not OK, feel free to DM me

[ancient roost]

I am here

@deep bronze So my Units have Skillsets with arrays of Skills and I want to add passive Skills such as critical damage, explosion on death, health loss when moving and others. Different Components controlling health, attack and movement are all represented as fields in Unit.
So as a result to add different kinds of listeners to these Components, Skills have to have references to Unit which leads to circular reference.

[deep bronze]

Others can also join the conversation and suggest something better than me, so there's a benefit too

[ancient roost]

And I use asmdefs so not having circular references is very important

For nowSkills and Units are in the same assembly but I wanted to move them apart

[deep bronze]

First-of-all, should you separate combat and skills logically? Is Skillset such a big feature and does it affect more systems than just combat? If there are skills planned to affect your dialogue trees or level up speed, etc., then it's reasonable to separate them. If they only affect combat, then I'd leave them in the combat assembly, in a separate folder.

Logically, skills affect health, defense and damage by changing their values, so they are dependent on combat features, and health doesn't need to know about skills and what exact skills affect it. It just has a public setter for HP or for the multiplier of HP, and each skill changes the multiplier in its own way.

Unit seems like a class that just knows about all the systems and has convenient fields to reference health, damage, etc. What's the purpose of this class? I don't mean to say that it doesn't have one but don't fall into the trap of creating a god object. If you implement AI in the future, will Unit have a field for the AI Brain too? Judging by what you said of Unit so far, it's a class for managing health and damage in a convenient way. Maybe rename it to something like CombatUnit so that it better encompasses its responsibility (remember the Single Responsibility Principle?). Then you can have a SkillsetOwner component that references CombatUnit to change its health and damage. This way you can have CombatUnits without skills. In fact, they don't need skills to start combat. They can just have pre-set health and damage values. But SkillsetOwners can't exist without also being CombatUnits. What's the reason of being SkillsetOwner if you don't participate in combat? So we can clearly see which way the dependency should go.

[ancient roost]

I think it would be reasonable to later add skills increasing passive money income, or even skills decreasing cooldowns of other skills

For now Unit has a Skillset, links to some Components such as Health, Walking, some methods like Die (which should be in Health tho) and Despawn and some methods to easily access the Components

Talking about skills decreasing cooldowns of other skills perhaps I should create another Component and call it kinda Casting which also would store a few variables such as cooldown decrease, skills damage amplification and maybe a few others

Also I am not sure what you mean by not falling into a god object as I am not too experienced and do not know what is bad in it and how to avoid it

[deep bronze]

God Object is a class that knows too much about other stuff and doesn't have a specific responsibility but rather can do everything. It's a code smell meaning it's a bad practice in code. You can read more about it here
https://www.c-sharpcorner.com/article/god-object-a-code-smell/

There are good practices in code abbreviated SOLID. https://www.c-sharpcorner.com/UploadFile/damubetha/solid-principles-in-C-Sharp/
The first principle is called Single Responsibility Principle. It says "Every software module should have only one reason to change". Jason Storey explained it wonderfully https://youtu.be/sh7f4K9Wbj8

After watching the video, ask yourself a question "What is the responsibility of my Unit class? If it's not just for combat, then does it know of too much? If I add a new system like AI, will I have to change my Unit class so that it knows about AI too?"

"I think it would be reasonable to later add skills increasing passive money income, or even skills decreasing cooldowns of other skills"

That's good to know. Then you actually have a reason to separate skills from the combat system. Skills will have to know (depend on) movement, combat, economy systems. And that's fine because that's their responsibility. But combat doesn't have to know of skills.

Your Unit class has to be split so that it doesn't know of Skillset, and renamed to CombatUnit or something else, or even removed completely! Since Health, DamageTaker, Walker are all components, you can have a skill component on the game object call GetComponent<Health>() to increase health or GetComponent<DamageDealer>() to increase damage.

What is a Unit conceptually? It's an object that has damage, health, skills. It looks to me like a GameObject called "Unit" that has DamageDealer and Health components. Unit doesn't have to be a separate component.

As for skills, you can have a catch-all component called Skillset that has an array of simple serializable classes derived from Skill class, or each Skill can be its own individual component, and if a unit needs a new skill, you can just call AddComponent<HealerSkill>(). It's up to you to develop the system further, but skills will be separated from the Unit class.

[ancient roost]

Thank you but can you please make adjustments to your advice keeping in mind that my skills are ScriptableObjects and I want them to have levels individual for each game session?

[deep bronze]

ScriptableObjects serve good as data templates. So it's convenient to create a ScriptableObject for your damage-increasing skill and define how much damage it will add per level. But the data individual to the player or game session has to be stored somewhere else because it isn't saved in ScriptableObjects when the game is quit. It needs to be loaded from your save-load system, and the method to load it depends on your implementation of the save-load system. You can start with PlayerPrefs and move from there to something more complex should you need it. So, you reference a ScriptableObject skill in the unit, then load the level of the skill from PlayerPrefs and apply it to the skill owned by this unit. This way you will get the individual values applied to this unit health or damage.

[ancient roost]

The reason I asked to make adjustments is because you mentioned getting Components in skills with GetComponent but it should be something else as ScriptableObjects do not have such function

Should it simply be GetComponent called on Skillset?

Wait, Skillsets should be ScriptableObjects too, as it would be convenient to have kits of skills packed...

[deep bronze]

Ah OK, got it. Honestly, it seems like overcomplication to make this skill system with ScriptableObjects. A skill is a concept that doesn't exist out there in the world but is connected to an owner i.e. a game object. It would be simpler to make them components. You can still find use for ScriptableObjects in this system as data components. So DamageIncreasingSkill is a component but DamageIncreasingSkillData is a scriptable object that holds data to how much damage it adds per level (it can be a part of DamageIncreasingSkill but whatever works best for you).

If you still want skills to be scriptable objects, they need to know which unit they affect to change that unit's health or damage. You have to interface units with their skills. By that, I mean you can pass the unit's game object to each skill when asking the skill to apply some adjustments to the unit's stats. The skill will then call GetComponent<> on the unit to change its values. Another way to do it is to pass the specific components the skill needs. So, you will pass the unit's Health component to the Healing skill, and DamageDealer component to the Berserker skill. Either way, you have to have a component that will connect the unit with its skills. You can call it SkillsUser. The SkillsUser component will live on units and will have an array of skills those units have. When the time comes to apply skills, SkillsUser will iterate through all the skills, pass the unit's game object to them and will ask them to change the unit's stats.

You can make a SkillSet scriptable object that will just hold an array of skills. SkillsUser, instead of referencing an array of skills, will reference a SkillSet object and will retrieve the array from there

[ancient roost]

I was thinking the same. Can I ask you further if I get any other problems with the topic?

[ancient roost]

@deep bronze I think you are very competent and helping can I ask you about another thing related to architecture?

[ancient roost]

I will post my question for case you answer positively. As usually context implies asmdefs.
I have Builder class and Grid class. Grid is created as a 2d array and Builder takes value from it to check whether you can build there and also shows some visual stuff and finally builds the Buildable.
I have Tower which inherits from Buildable which inherits from MonoBehaviour. I am going to add to Tower an enumeration which would determine the tower's rarity. The enumeration will be used to determine the price we sell the tower for with a skill and which passive skill will be added to the tower to determine the tower's rarity (only sprite will be changed). But I think the skill should be added at runtime so I get question how should I add it. Was thinking of adding the skill at the moment of building but it's not something Builder should do and also Builder can't reference skills as it would create a circular reference (as there are skills such as "place pumpkin"). Actually, even writing this text helped me 1-3 times and now I think of creating an event in builder which will be called on building and listening to this event from a class in the same asmdef with Tower which will check if the object being built is a tower and then add skills but the problem is that there is a skill that is supposed to sell the tower so we get a circular reference again...

Knowing that there is a skill selling towers we can say that tower asmdef is a dependency for skills asmdef so why not creating the listening class in the skills asmdef? Seems like a solution to me

Please answer me, I think your answer will be very useful for me. Especially interested in your comments to enumeration violating SOLID-O and the final solution

[sand scarab]

why do you need a skill to buy/sell the tower? wouldn't that just be an option when selecting the tower (to sell) and from a menu/store (to buy)?

[sand scarab]

maybe i'm missing smth but don't understand why there's a passive skill to add tower rarity. wouldn't you assign a field when the tower is created and access it via property? don't add components at runtime. they should already exist on the object and you'd enable it if needed . . .

[ancient roost]

The passive skill is just to visually show the rarity

[ancient roost]

I want it to be a skill

You can consider it part of game's design

[ancient roost]

But damm, I think I really need just to add them

At editor

But damm, it would be way better if they were added at runtime based on their type/enum

Cuz it makes it easier to change tower's level if needed

[sand scarab]

if it's visual it'd just be text on the ui which you grab from the Rarity property on the tower . . .

[ancient roost]

Man I am copying a custom play mode from dota 2

And there the same feature is considered a skill

So I want it to be a skill too

[sand scarab]

i have a hard time understand what skills mean to you as that is just displaying a value, which is not a skill . . .

[ancient roost]

Actually, I I think my skills&items system is pretty advanced

[sand scarab]

it's just like displaying health. when the value changes you update it. same applies to the tower's level . . .

[ancient roost]

Everything also has a tooltip

So you can add a skill just to show some info

[sand scarab]

huh? a tooltip would access the tower class to display any of its properties. why is that a skill?

[ancient roost]

You will understand what skills mean to me if you imagine dota 2

[ancient roost]

Part of game's design as I said

There won't be any tooltip after click on tower

I mean

There will only be displayed skills, items, hp, and tower's picture

But it's on the bottom part of screen

Let me show you

Like this @sand scarab

You can see skills in the middle part

And items in the right part

Of the character panel

And having a passive skill which simply provides info about tower's rarity is a part of game's design

Oh, let me show you the tooltips

[ancient roost]

And even if I abandon skill showing tower's rarity there anyway is an item which will give the tower a special item. And items use skills inside

The thing is that seems like semantically my tower and skills depend on each other, it is wrong

Omg, I can simply create the class listening to Builder in skills asmdef

Programming is so beautiful

But it also doesn't seem too good semantically

Then I can create a separate asmdef for this

You can consider this place my devlog

[sand scarab]

It just seems confusing and hard to grasp how your architecture is setup (without code), especially to interact with one another. You should separate dependencies. Using events is a safe way to interact with other systems as it doesn't break anything when you (un)sub from an event . . .

[ancient roost]

Of course my dependencies are separated

Like, I have many asmdefs

Perhaps I will make a picture soon and post it here

[ancient roost]

But I don't think there is a lot to show since I've found the solution

The one I mentioned here

[sand scarab]

As long as it works, right? Also, you don't need an asmdef for everything, namely separate systems or namespaces . . .

[ancient roost]

No, I need...

To let them work together

And to force my self to have better code

[ancient roost]

I still was pondering whether it is a good solution and I think I've come up with an even better one: as these skills and the item are almost integral parts of the tower, they should be stored in the same asmdef

This way the tower asmdef will depend on skills and not backwards

[ancient roost]

Megabrain

This way may not even create an enumeration and other excess stuff and store the needed skill directly inside the tower's class

And not violate SOLID-O

[sand scarab]

The asmdef doesn't really matter. That's just for splitting up scripts into groups (dependencies) and helps with compiling; It's not a requirement. You can create one after you sort all the scripts out . . .

[ancient roost]

I guess it doesn't really matter but having asmdefs is a good thing

[sand scarab]

Oh, I have them as well; it helps with compile times when updating scripts . . .