Move the CCmpTemplateManager inside the ComponentManager
Needs RevisionPublic
Actions

Authored by phosit on Jul 24 2023, 8:00 PM.

Details

Reviewers

Itms

Summary

Stores the CCmpTemplateManager in a tuple so that all C++ system components can be stored near each other.

Baciacly another version of this. There are differences:

The ConstructComponent is split up. Instead of using a conditional.
The tests should pass
The serialization should works (didn't test rejoin)

Test Plan

Test the (de-)serialization: multiplayer, loading saved games

Diff Detail

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

phosit requested review of this revision.Jul 24 2023, 8:00 PM

phosit created this revision.

The CCmpTemplateManager is special the other components will be simpler to port ^^

source/simulation2/components/CCmpTemplateManager.h
39	Should there be a `ENSURE(false)`? The caller does also `ENSURE`.
source/simulation2/system/ComponentManager.h
77	Now I'm more in favor of using a forward declared struct instead of a tuple. Then only one type would have to be forward declared.
source/simulation2/system/ComponentManagerSerialization.cpp
332–340	Getting the `IComponent*` from a `CID` will be usefull in other places as well. Making a lookup function for that might be worth it. (It can't be implemented in terms of query since with query they have to be "attached" but query could be implemented in terms of the lookup.)
source/simulation2/tests/test_CmpTemplateManager.h
60	Why isn't `InitSystemEntity` called in the constructor?
64	We can't provide ower own `CEntityHandle` for system components. Is that a problem?

phosit added a parent revision: D5086: Split header and source of CCmpTemplateManager.Jul 24 2023, 9:43 PM

phosit mentioned this in D4842: [POC] Static SystemComponents.

Use a forward declared struct.
Add REGISTER_STATIC_COMPONENT_TYPE macro.

gah, I was not reactive enough to your message on IRC. I don't think the check for an existing instance of the component prevents you from decoupling construction and attachment of the component. Instead, I suggest you move the check to the component attachment. Maybe another check can be added somewhere else to reduce the risks of memory leaks in future modifications of the code.

I am leaving comments for additional improvements to the methods of the ComponentManager. There are a couple methods that could be streamlined wrt the construct/attach/initialize pattern.

Apart from those improvements, I am fully approving the change to static system components and I am going to test the branch 🙂 Nice work!

source/simulation2/system/ComponentManager.cpp
747–748	So I think this can be safely moved to the component attachment. If you disagree, if (m_ComponentsByInterface[ct.iid].find(ent.GetId()) != m_ComponentsByInterface[ct.iid].end()) looks short enough for me, and if you really want to keep it in two lines, please rename it to `emap` now that there is no `emap2` 😉
782	Move the check here that `ent` does not already contain an component of type `cid`, return early if it does. Additionally, you could add a duplicate version of the check in the caller(s) of `AttachComponent` (after all, the caller already has `ent` and `cid`). If that doesn't sound safe enough, you can always make this function return an explicit `bool` and the caller should destroy the constructed component if this returns `false`.
790–794	The decoupling would make this easier, so maybe that comment can be amended to better match the new state of the code.
source/simulation2/system/ComponentManager.h
190	`ent` can become `const` like in the new methods.
196–197	style nitpick: I don't think the line break is needed (other lines in this file are longer)
210	This should become `AttachMockComponent` since it is now very similar to `AttachComponent`. `ent` can be `const`. I suggest you rename it and move this declaration after `AttachComponent`, to match the order of implementations in the `.cpp` file.
213	Don't forget to remove the second part of that sentence if you decouple attachment from construction.
217	`ent` can be removed if you decouple attachment from construction, else it can be `const`.
220	I would copy-paste the second part of the `ConstructComponent` comment. * (The component's Init is not called here - either Init or Deserialize must be called * before using the returned object.)
222	I suggest changing the signature to match better the other methods, especially `Add`(`Attach`)`MockComponent`: void AttachComponent(const CEntityHandle ent, const ComponentTypeId cid, IComponent& component);
source/simulation2/system/ComponentManagerSerialization.cpp
332	I think this must be `m_SystemComponents->GetComponentByCid(ctid);`. That doesn't make a difference in this commit, but I think you forgot it for the chain of migration for the other components.

This revision now requires changes to proceed.Jul 30 2023, 7:01 PM

Stan added a subscriber: Stan.Jul 31 2023, 8:54 AM

Stan added inline comments.

source/simulation2/components/CCmpTemplateManager.h
36	int -> ComponentTypeId ?

Itms added inline comments.Jul 31 2023, 9:38 AM

source/simulation2/components/CCmpTemplateManager.h
36	No, already discussed, `ComponentTypeId` is a `typedef` internal to the component manager. Also the macro uses `int`, so keep that.

In D5088#216443, @Itms wrote:

I don't think the check for an existing instance of the component prevents you from decoupling construction and attachment of the component. Instead, I suggest you move the check to the component attachment.

Splitting the "owning" (m_ComponentsByTypeId) and the "construction-/destruction-knowledge" (m_ComponentTypesById), might had been a good idea. Now that there is a thing that does own and knows how to construct / destruct components (m_SystemComponents) the border is blurred.

Maybe another check can be added somewhere else to reduce the risks of memory leaks in future modifications of the code.

Every call to AttachComponent would be followed by a if (!attached) DestroyComponent(pComponent).

The concept of "insert if not existing" is a common idiom for associative containers (std::map::insert).

Propably for after the transition: Now that there are multiple ways to allocate components it might be a good idea to make them satisfy the Allocator named requirement.