Event Timeline
(As mentioned in the inline comments actually passing cmpOld to MoveFrom works really well for JS components. However it doesn't work at all for C++ components and having the latter as a special case is ugly and suddenly requires at least some code to care if a component (interface) is in C++ or not. Adding conversions for components seems like a very ugly thing to do, so just passing the entity id might be slightly awkward (especially if we just query for the same component after having done just that), but I didn't find a better solution for this back then. Maybe something could be done with optional parameters.
Mostly this is a WIP/RFC kind of thing.
Does look much nicer. I'm wondering if we shouldn't act like it's a copy rather than a move though.
How much slower is it actually to just require each cpp entity component to have an exposed CopyFromEnt / MoveFromEnt method (which could default to an inherited void void)?
I mostly named it move as opposed to copy since the use case we have for it will destroy the previous entity and component (and move captures those semantics better).
About requiring each component: I don't think we should do that, but exposing a function of some name and then checking for && cmpNew.FooBar before calling cmpNew.FooBar(ent) should be doable.
(Sure we don't currently transform lots of things, but I'd rather avoid JS->C++->virtual calls if we know that we aren't going to do any work. Also quite a few components just don't have any state that should be carried over.)
I mostly named it move as opposed to copy since the use case we have for it will destroy the previous entity and component (and move captures those semantics better).
Indeed, but I'm wondering if that would always be the case, as in "should the functions take care to leave the original in a valid state or not".
I'm not too sure about the performance here, but it might be worth making this a system function instead (as in a ComponentManager function) that can use the component cache to dispatch the calls. Would save on the QueryInterface costs, we'd just need to add a generic does-nothing version to Component. That might end up being slower if we only copy from few components though.
Re CanGarrisonedChangeTemplate, I think it's trying to figure out if the units garrisoned in "Old" are valid to garrison in "New". Garrisoning is honestly a mess though :/
Since I haven't been able to come up with a case where we'd care about the original state afterwards I opted for the "we don't care what state it is left in afterwards" option which is move. If someone has an example where valid state would be nice to have (and still everything is copied/moved to the new entity) I'd be interested in hearing that. Also most (all) of the MoveFrom functions should work by just using interface methods (to also support changing to a different implementation of that interface), so it actually is Copy. (As said naming is not set in stone, but written in the sand and the tide is coming in.)
Given that not a lot of code uses this currently I'm not sure if we should care that much. Moving it to the ComponentManager would add at least one virtual function call per interface that both the old and new entity share. And I think we will not want to copy all components, since even right now we only take care of a few (which seems to work ok). I guess we can still change to that if it turns out that performance is needed here and that moving it to the ComponentManager (or similar) helps.
Yes, but it breaks for at least visually garrisoned entities. It also seems to do something related to transforming garrsioned entities, and complains about not having any information on the current garrisonholder, which shouldn't be that hard to get since IIRC we do handle garrisoned heroes (and focussing on them). Basically half thought out code that exists for some reason while it would most likely be nicer if it just didn't (since a proper fix will end up replacing most of it anyway).
Since we send the message entity renamed already, can't we just let components handle renaming themselves?
Then Transform.js only needs to create a new entity, send the EntityRenamed-message and destroy the old entity.
(Okay perhaps we need to set some stuff (e.g. Ownership) before others.)
Well that's the point, why we can't really use the messages. The sender of the message has no clue what the components will do with it. Also this message triggers the globalEntityRenames. So if we were still transferring data on the message, other entities might see a partially transferred ent (now I am not entirely sure of the order of the global vs local messages, but we shouldn't rely on that order in the first place). Also we simply don't know in which order the components are called. As we see some components depend on others, so we always will need to enforce some order.
So if we were to strip the transform function apart to the components (which makes sense, since why would "transform" make any assumptions on what components there are?) we need the following two things:
- A new type of message (call it MoveFrom as in this paste, or onTransform, or whatever makes sense) which components can subscribe to, to set the data they need
- Some recursive loop to make sure all components can what on some other component if they wish to. Feel free to take inspiration from D4231 on this.
Using the EntityRenamed is wrong to use, if we are still renaming ("Renamed" has a passed tense!!). So we should reserve that for calling after all the transfer has occurred.