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

Update ecs.md #83

Closed
wants to merge 1 commit into from
Closed

Conversation

AlbertSnows
Copy link
Contributor

I edited the TODO section of the ECS chapter that contained commentary on OOP criticisms to "clean up" the text and have it flow in a (hopefully) more readable style.

Fixes #82

I edited the TODO section of the ECS chapter that contained commentary on OOP criticisms to "clean up" the text and have it flow in a (hopefully) more readable style.
Copy link
Contributor

@Partmedia Partmedia left a comment

Choose a reason for hiding this comment

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

Thank you for working on these docs. I've left a few comments for you.


Edit all this text into something proper for a doc... Sigh
Let's first take a look at inheritance. There are many problems and inflexibility that come with complex inheritance trees. Refer to the tree diagram above, and imagine we had no components. Instead, we have large inheritance tree. In this tree, we have a "machine" base class that has power consumption. We also have a different "mob" base class that has various player-controlled mechanics. Notice that both of these objects are on two separate branches of our component hierarchy. This poses our first design dilemma; what if we want some machines to have mob qualities, or some mobs to have machine qualities? Our system is designed in such a manner where you cannot make a mob have machine qualities or vice versa without making ugly hacks, or a common base for both "machine" and "mob".
Copy link
Contributor

Choose a reason for hiding this comment

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

This duplicates a lot of what is already written starting on line 18. It would be better to work any additions you have into the existing text instead of repeating it.

I'm more saying like... Rather than have logic in components change stuff, we want entity systems to operate and modify components. So instead of having:
`HandsComponent.Pickup(ItemComponent)`, you would have:
`HandsSystem.Pickup(UserEntity, ItemEntity)`.
Inheritance may be perfectly fine in nuanced circumstances, such as small, self-contained inheritance trees (see something like `SoundSpecifier` for example). However, at scale its benefits become detriments quickly. Especially when you have a game as large and complex as ss14.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really about scale, it's about the object-oriented model running into problems modeling in-game data objects. It may be helpful to link external resources, too.


Now let's take a look at encapsulation. OOP associates data and behavior in the same class. Such classes only expose certain properties or methods to external parties via access modifiers. That is, fields such as `public`, `private` and `protected`. Encapsulation is good for cases such as our toolbax engine; which specifically needs to obscure/prohibit access to some data or methods. But for game and data logic, it doesn't make that much sense. For example, the `StackComponent` in SS14 has no private fields or properties and any owner is free to read or write the values as they please. However, this is not the recommended way to interact with stacks!
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you respond to the question, "it sounds like SS14 is using access modifiers wrong by not hiding private internal data fields?" OOP is also about encapsulation. What is different between StackSystem.Split() vs a stackInstance.Split() method?


`StackSystem` has a few methods to operate on `StackComponent` and change its values. To use a certain amount of things on the stack, you use `StackSystem.Use`. To split the stack, `StackSystem.Split`; and so on. `StackSystem` acts as the orchestrator, and is responsible for acting on this component to express behavior. For something as simple as splitting a stack, this may seem like overkill. That is until you consider that as our features grow in complexity, changing the stack amount value isn't enough. As requirements expand, we will also need to accomplish additional tasks. For example, dirtying the component for network syncing purposes, setting the appearance value, raise a `StackCountChanged` event, and more.

The way to accomplish this in E/C (a similar, alternative architecture) would be to put all of that logic in an "amount" property on `StackComponent` with an access method. Then you could `private` the actual "amount" number away. However, putting logic of any kind in a component class brings us back to the troubles of OOP as was mentioned prior. Take a step back for a moment to think about what it is, exactly, that we're bulding. Our game system is built on two principle concepts: entities and systems. Entities are composed of components. Systems transform components. As such, systems transform entities. If we need to express a *change* in the state of our game, that's what our systems are for. Components should only have data in them, and no logic whatsoever. It is our systems that should give the entities their behavior, when they have the appropriate components.
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be helpful to explain why. This explanation is not quite it since we don't in fact private the actual "amount" number away.

@AlbertSnows
Copy link
Contributor Author

Thank you for working on these docs. I've left a few comments for you.

I will try to address these when I have time, or someone else can beat me to it.

@Jezithyr Jezithyr closed this Mar 4, 2024
@Jezithyr
Copy link
Contributor

Jezithyr commented Mar 4, 2024

Closed as part of the docs repo cleanup. If you are still actively working on this PR feel free to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address the TODO OOP criticism section
3 participants