Page MenuHomeWildfire Games

Possibly nicer approach for replacing one entity's component values with that of another (aka Transform/Upgrade/Replace)
ActivePublic

Authored by leper on May 19 2017, 7:27 PM.
Tags
None
Subscribers
Restricted Owners Package
Restricted Owners Package
Tokens
"Mountain of Wealth" token, awarded by elexis.
From 723007d4c26a70737a46ceff5d05e66af79f5aeb Mon Sep 17 00:00:00 2001
From: leper <leper@wildfiregames.com>
Date: Sat, 31 Dec 2016 17:23:20 +0100
Subject: [PATCH] TODO cmpUpgrade and related.
---
.../public/simulation/components/Capturable.js | 9 ++
.../public/simulation/components/GarrisonHolder.js | 17 ++++
.../mods/public/simulation/components/Guard.js | 16 ++++
.../mods/public/simulation/components/Health.js | 7 ++
.../mods/public/simulation/components/UnitAI.js | 24 +++++
.../mods/public/simulation/helpers/Commands.js | 7 +-
.../mods/public/simulation/helpers/Transform.js | 106 ++++-----------------
source/simulation2/components/CCmpOwnership.cpp | 9 ++
source/simulation2/components/ICmpOwnership.cpp | 1 +
source/simulation2/components/ICmpOwnership.h | 2 +
10 files changed, 109 insertions(+), 89 deletions(-)
diff --git a/binaries/data/mods/public/simulation/components/Capturable.js b/binaries/data/mods/public/simulation/components/Capturable.js
index c81be89433..a42044319a 100644
--- a/binaries/data/mods/public/simulation/components/Capturable.js
+++ b/binaries/data/mods/public/simulation/components/Capturable.js
@@ -18,6 +18,15 @@ Capturable.prototype.Init = function()
this.cp = [];
};
+Capturable.prototype.MoveFrom = function(oldEnt)
+{
+ // Rescale capture points
+ let cmpOld = Engine.QueryInterface(oldEnt, IID_Capturable);
+ let scale = cmpOld.GetMaxCapturePoints() / this.GetMaxCapturePoints();
+ let newCp = cmpOld.GetCapturePoints().map(v => v / scale);
+ this.SetCapturePoints(newCp);
+};
+
//// Interface functions ////
/**
diff --git a/binaries/data/mods/public/simulation/components/GarrisonHolder.js b/binaries/data/mods/public/simulation/components/GarrisonHolder.js
index 8b8e02c363..33c1373b25 100644
--- a/binaries/data/mods/public/simulation/components/GarrisonHolder.js
+++ b/binaries/data/mods/public/simulation/components/GarrisonHolder.js
@@ -78,6 +78,23 @@ GarrisonHolder.prototype.Init = function()
}
};
+GarrisonHolder.prototype.MoveFrom = function(oldEnt)
+{
+ var cmpOld = Engine.QueryInterface(oldEnt, IID_GarrisonHolder);
+ if (!cmpOld.GetEntities().length)
+ return;
+
+ var garrisonedEntities = cmpOld.GetEntities().slice();
+ for (let ent of garrisonedEntities)
+ {
+ let cmpUnitAI = Engine.QueryInterface(ent, IID_UnitAI);
+ cmpOld.Eject(ent);
+ // TODO Only Autogarrison if Garrison() succeeds
+ cmpUnitAI.Autogarrison(newEnt);
+ this.Garrison(ent);
+ }
+};
+
/**
* Return range at which entities can garrison here
*/
diff --git a/binaries/data/mods/public/simulation/components/Guard.js b/binaries/data/mods/public/simulation/components/Guard.js
index dca201f8c7..e3a0203757 100644
--- a/binaries/data/mods/public/simulation/components/Guard.js
+++ b/binaries/data/mods/public/simulation/components/Guard.js
@@ -8,6 +8,22 @@ Guard.prototype.Init = function()
this.entities = [];
};
+Guard.prototype.MoveFrom = function(oldEnt)
+{
+ let cmpOld = Engine.QueryInterface(oldEnt, IID_Guard);
+ let entities = cmpOld.GetEntities();
+ if (!entities.length)
+ return;
+
+ this.SetEntities(entities);
+ for (let ent of entities)
+ {
+ let cmpEntUnitAI = Engine.QueryInterface(ent, IID_UnitAI);
+ if (cmpEntUnitAI)
+ cmpEntUnitAI.SetGuardOf(this.entity);
+ }
+};
+
Guard.prototype.GetRange = function(entity)
{
let range = 8;
diff --git a/binaries/data/mods/public/simulation/components/Health.js b/binaries/data/mods/public/simulation/components/Health.js
index f3f2445dee..9aee747201 100644
--- a/binaries/data/mods/public/simulation/components/Health.js
+++ b/binaries/data/mods/public/simulation/components/Health.js
@@ -68,6 +68,13 @@ Health.prototype.Init = function()
this.UpdateActor();
};
+Health.prototype.MoveFrom = function(oldEnt)
+{
+ var cmpOld = Engine.QueryInterface(oldEnt, IID_Health);
+ var healthLevel = Math.max(0, Math.min(1, cmpOld.GetHitpoints() / cmpOld.GetMaxHitpoints()));
+ this.SetHitpoints(Math.round(this.GetMaxHitpoints() * healthLevel));
+};
+
//// Interface functions ////
/**
diff --git a/binaries/data/mods/public/simulation/components/UnitAI.js b/binaries/data/mods/public/simulation/components/UnitAI.js
index 7efba14b4e..442cece96e 100644
--- a/binaries/data/mods/public/simulation/components/UnitAI.js
+++ b/binaries/data/mods/public/simulation/components/UnitAI.js
@@ -3346,6 +3346,30 @@ UnitAI.prototype.Init = function()
this.SetStance(this.template.DefaultStance);
};
+UnitAI.prototype.MoveFrom = function(oldEnt)
+{
+ let cmpOld = Engine.QueryInterface(oldEnt, IID_UnitAI);
+ let pos = cmpOld.GetHeldPosition();
+ if (pos)
+ this.SetHeldPosition(pos.x, pos.z);
+
+ if (cmpOld.GetStanceName())
+ this.SwitchToStance(cmpOld.GetStanceName());
+
+ this.AddOrders(cmpOld.GetOrders());
+
+ if (cmpOld.IsGuardOf())
+ {
+ let guarded = cmpOld.IsGuardOf();
+ let cmpGuard = Engine.QueryInterface(guarded, IID_Guard);
+ if (cmpGuard)
+ {
+ cmpGuard.RenameGuard(oldEnt, newEnt);
+ this.SetGuardOf(guarded);
+ }
+ }
+};
+
UnitAI.prototype.IsTurret = function()
{
if (!this.IsGarrisoned())
diff --git a/binaries/data/mods/public/simulation/helpers/Commands.js b/binaries/data/mods/public/simulation/helpers/Commands.js
index 5860b44f0d..ae137e4776 100644
--- a/binaries/data/mods/public/simulation/helpers/Commands.js
+++ b/binaries/data/mods/public/simulation/helpers/Commands.js
@@ -680,10 +680,11 @@ var g_Commands = {
"upgrade": function(player, cmd, data)
{
+ var template = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager).GetTemplate(cmd.template);
+ var cmpEntityLimits = QueryPlayerIDInterface(player, IID_EntityLimits);
for (let ent of data.entities)
{
var cmpUpgrade = Engine.QueryInterface(ent, IID_Upgrade);
-
if (!cmpUpgrade || !cmpUpgrade.CanUpgradeTo(cmd.template))
continue;
@@ -708,9 +709,6 @@ var g_Commands = {
}
// Check entity limits
- var cmpTemplateManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager);
- var template = cmpTemplateManager.GetTemplate(cmd.template);
- var cmpEntityLimits = QueryPlayerIDInterface(player, IID_EntityLimits);
if (template.TrainingRestrictions && !cmpEntityLimits.AllowedToTrain(template.TrainingRestrictions.Category, 1) ||
template.BuildRestrictions && !cmpEntityLimits.AllowedToBuild(template.BuildRestrictions.Category))
{
@@ -719,6 +717,7 @@ var g_Commands = {
continue;
}
+ // TODO: Do we really want to support multiple owners for the units we give this command to, just filter data.entities, and we can move this one level up
var cmpTechnologyManager = QueryOwnerInterface(ent, IID_TechnologyManager);
if (cmpUpgrade.GetRequiredTechnology(cmd.template) && !cmpTechnologyManager.IsTechnologyResearched(cmpUpgrade.GetRequiredTechnology(cmd.template)))
{
diff --git a/binaries/data/mods/public/simulation/helpers/Transform.js b/binaries/data/mods/public/simulation/helpers/Transform.js
index 77dcad1191..fc2b08cf56 100644
--- a/binaries/data/mods/public/simulation/helpers/Transform.js
+++ b/binaries/data/mods/public/simulation/helpers/Transform.js
@@ -1,5 +1,8 @@
// Helper functions to change an entity's template and check if the transformation is possible
+// TODO we need tests for this, so we can be somewhat sure that this thing actually works
+// which it didn't for at least guard...
+
// returns the ID of the new entity or INVALID_ENTITY.
function ChangeEntityTemplate(oldEnt, newTemplate)
{
@@ -11,6 +14,22 @@ function ChangeEntityTemplate(oldEnt, newTemplate)
return INVALID_ENTITY;
}
+ // TODO should we do this for all components, and require this function to be present everywhere?
+ // This seems like it would be slow even if we only need a few components (maybe make them
+ // register themselves, and only then handle them here)
+ for (let iid of [/*IID_Position, IID_Obstruction,*/IID_Ownership, IID_Capturable, IID_Health, IID_UnitAI, IID_Guard, IID_GarrisonHolder])
+ {
+ let cmpOld = Engine.QueryInterface(oldEnt, iid);
+ let cmpNew = Engine.QueryInterface(newEnt, iid);
+ if (cmpOld && cmpNew)
+ // TODO docs: MoveFrom can assume that cmpOld exists (so no need to check)
+ cmpNew.MoveFrom(oldEnt);
+ // TODO for JS components just passing cmpOld works nicely (but might still lead to people trying to do that elsewhere (and then storing it...)
+ // HOWEVER passing that to C++ seems to be a bit more involved,
+ // AND requiring user code to know whether a component is C++ (or c++ interface) or JS is bad.
+ // And not using the same way for C++ components has sort of the same problem, and is a bit ugly.
+ }
+
var cmpPosition = Engine.QueryInterface(oldEnt, IID_Position);
var cmpNewPosition = Engine.QueryInterface(newEnt, IID_Position);
if (cmpPosition && cmpNewPosition)
@@ -26,75 +45,9 @@ function ChangeEntityTemplate(oldEnt, newTemplate)
cmpNewPosition.SetHeightOffset(cmpPosition.GetHeightOffset());
}
- var cmpOwnership = Engine.QueryInterface(oldEnt, IID_Ownership);
- var cmpNewOwnership = Engine.QueryInterface(newEnt, IID_Ownership);
- if (cmpOwnership && cmpNewOwnership)
- cmpNewOwnership.SetOwner(cmpOwnership.GetOwner());
-
// Copy control groups
CopyControlGroups(oldEnt, newEnt);
- // Rescale capture points
- var cmpCapturable = Engine.QueryInterface(oldEnt, IID_Capturable);
- var cmpNewCapturable = Engine.QueryInterface(newEnt, IID_Capturable);
- if (cmpCapturable && cmpNewCapturable)
- {
- let scale = cmpCapturable.GetMaxCapturePoints() / cmpNewCapturable.GetMaxCapturePoints();
- let newCp = cmpCapturable.GetCapturePoints().map(v => v / scale);
- cmpNewCapturable.SetCapturePoints(newCp);
- }
-
- // Maintain current health level
- var cmpHealth = Engine.QueryInterface(oldEnt, IID_Health);
- var cmpNewHealth = Engine.QueryInterface(newEnt, IID_Health);
- if (cmpHealth && cmpNewHealth)
- {
- var healthLevel = Math.max(0, Math.min(1, cmpHealth.GetHitpoints() / cmpHealth.GetMaxHitpoints()));
- cmpNewHealth.SetHitpoints(Math.round(cmpNewHealth.GetMaxHitpoints() * healthLevel));
- }
-
- var cmpUnitAI = Engine.QueryInterface(oldEnt, IID_UnitAI);
- var cmpNewUnitAI = Engine.QueryInterface(newEnt, IID_UnitAI);
- if (cmpUnitAI && cmpNewUnitAI)
- {
- let pos = cmpUnitAI.GetHeldPosition();
- if (pos)
- cmpNewUnitAI.SetHeldPosition(pos.x, pos.z);
- if (cmpUnitAI.GetStanceName())
- cmpNewUnitAI.SwitchToStance(cmpUnitAI.GetStanceName());
- cmpNewUnitAI.AddOrders(cmpUnitAI.GetOrders());
- if (cmpUnitAI.IsGuardOf())
- {
- let guarded = cmpUnitAI.IsGuardOf();
- let cmpGuard = Engine.QueryInterface(guarded, IID_Guard);
- if (cmpGuard)
- {
- cmpGuard.RenameGuard(oldEnt, newEnt);
- cmpNewUnitAI.SetGuardOf(guarded);
- }
- }
- }
-
- // Maintain the list of guards
- let cmpGuard = Engine.QueryInterface(oldEnt, IID_Guard);
- let cmpNewGuard = Engine.QueryInterface(newEnt, IID_Guard);
- if (cmpGuard && cmpNewGuard)
- {
- let entities = cmpGuard.GetEntities();
- if (entities.length)
- {
- cmpNewGuard.SetEntities(entities);
- for (let ent of entities)
- {
- let cmpEntUnitAI = Engine.QueryInterface(ent, IID_UnitAI);
- if (cmpEntUnitAI)
- cmpEntUnitAI.SetGuardOf(newEnt);
- }
- }
- }
-
- TransferGarrisonedUnits(oldEnt, newEnt);
-
Engine.BroadcastMessage(MT_EntityRenamed, { "entity": oldEnt, "newentity": newEnt });
Engine.DestroyEntity(oldEnt);
@@ -103,9 +56,10 @@ function ChangeEntityTemplate(oldEnt, newTemplate)
}
function CanGarrisonedChangeTemplate(ent, template)
+// TODO what is this doing, or even trying to do?
{
var cmpPosition = Engine.QueryInterface(ent, IID_Position);
- var unitAI = Engine.QueryInterface(ent, IID_UnitAI);
+ var unitAI = Engine.QueryInterface(ent, IID_UnitAI); // TODO cmpUnitAI
if (cmpPosition && !cmpPosition.IsInWorld() && unitAI && unitAI.IsGarrisoned())
{
// We're a garrisoned unit, assume impossibility as I've been unable to find a way to get the holder ID.
@@ -213,24 +167,6 @@ function DeleteEntityAndReturn(ent, cmpPosition, position, angle, cmpNewPosition
return ret;
}
-function TransferGarrisonedUnits(oldEnt, newEnt)
-{
- // Transfer garrisoned units if possible, or unload them
- var cmpOldGarrison = Engine.QueryInterface(oldEnt, IID_GarrisonHolder);
- var cmpNewGarrison = Engine.QueryInterface(newEnt, IID_GarrisonHolder);
- if (!cmpNewGarrison || !cmpOldGarrison || !cmpOldGarrison.GetEntities().length)
- return; // nothing to do as the code will by default unload all.
-
- var garrisonedEntities = cmpOldGarrison.GetEntities().slice();
- for (let ent of garrisonedEntities)
- {
- let cmpUnitAI = Engine.QueryInterface(ent, IID_UnitAI);
- cmpOldGarrison.Eject(ent);
- cmpUnitAI.Autogarrison(newEnt);
- cmpNewGarrison.Garrison(ent);
- }
-}
-
Engine.RegisterGlobal("ChangeEntityTemplate", ChangeEntityTemplate);
Engine.RegisterGlobal("CanGarrisonedChangeTemplate", CanGarrisonedChangeTemplate);
Engine.RegisterGlobal("ObstructionsBlockingTemplateChange", ObstructionsBlockingTemplateChange);
diff --git a/source/simulation2/components/CCmpOwnership.cpp b/source/simulation2/components/CCmpOwnership.cpp
index 68a8e8f36f..7961df5d86 100644
--- a/source/simulation2/components/CCmpOwnership.cpp
+++ b/source/simulation2/components/CCmpOwnership.cpp
@@ -77,6 +77,15 @@ public:
}
}
+ virtual void MoveFrom(entity_id_t oldEnt)
+ {
+ CmpPtr<ICmpOwnership> cmpOld(GetSimContext(), oldEnt);
+ if (!cmpOld)
+ return;
+
+ SetOwner(cmpOld->GetOwner());
+ }
+
virtual player_id_t GetOwner() const
{
return m_Owner;
diff --git a/source/simulation2/components/ICmpOwnership.cpp b/source/simulation2/components/ICmpOwnership.cpp
index debe2cb3a4..459ca0177a 100644
--- a/source/simulation2/components/ICmpOwnership.cpp
+++ b/source/simulation2/components/ICmpOwnership.cpp
@@ -22,6 +22,7 @@
#include "simulation2/system/InterfaceScripted.h"
BEGIN_INTERFACE_WRAPPER(Ownership)
+DEFINE_INTERFACE_METHOD_1("MoveFrom", void, ICmpOwnership, MoveFrom, entity_id_t)
DEFINE_INTERFACE_METHOD_CONST_0("GetOwner", player_id_t, ICmpOwnership, GetOwner)
DEFINE_INTERFACE_METHOD_1("SetOwner", void, ICmpOwnership, SetOwner, player_id_t)
DEFINE_INTERFACE_METHOD_1("SetOwnerQuiet", void, ICmpOwnership, SetOwnerQuiet, player_id_t)
diff --git a/source/simulation2/components/ICmpOwnership.h b/source/simulation2/components/ICmpOwnership.h
index 9e253a255a..93bd07802f 100644
--- a/source/simulation2/components/ICmpOwnership.h
+++ b/source/simulation2/components/ICmpOwnership.h
@@ -29,6 +29,8 @@
class ICmpOwnership : public IComponent
{
public:
+ virtual void MoveFrom(entity_id_t oldEnt) = 0;
+
virtual player_id_t GetOwner() const = 0;
virtual void SetOwner(player_id_t playerID) = 0;
--
2.13.0

Event Timeline

leper created this paste.May 19 2017, 7:27 PM
leper created this object with visibility "Public (No Login Required)".

(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.

Really pleasant to read.

It would make that kind of thing #4334 easier.

Does look much nicer. I'm wondering if we shouldn't act like it's a copy rather than a move though.

elexis added a subscriber: elexis.May 26 2017, 12:48 PM

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 am not good at naming and I'm fine with Move but Transfer could be ok too)

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).

(I just noticed there was a todo about that in Foundation component)

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.)

bb added a subscriber: bb.Oct 1 2021, 9:45 PM

(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.