Page MenuHomeWildfire Games

Fix rotation of promoted turrets
ClosedPublic

Authored by mimo on Mar 28 2018, 10:37 PM.

Details

Summary

When a turret is promoted, the y rotation set to the new entity should anticipate that it will be a turret, and then be defined relative to the turret angle.

Test Plan

Try to garrison sone units on wall, and promote them.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mimo created this revision.Mar 28 2018, 10:37 PM
mimo updated this revision to Diff 6285.Mar 28 2018, 10:40 PM
mimo edited the summary of this revision. (Show Details)

add a comment

temple added a subscriber: temple.Mar 29 2018, 9:19 PM

I think it's better to set when setting the turrent parent, as you suggested earlier.

Also to stop the turning while garrisoning we should do SetFacePointAfterMove(false) from UnitMotion.

I mean something like this:

Index: binaries/data/mods/public/simulation/components/GarrisonHolder.js
===================================================================
--- binaries/data/mods/public/simulation/components/GarrisonHolder.js	(revision 21631)
+++ binaries/data/mods/public/simulation/components/GarrisonHolder.js	(working copy)
@@ -201,6 +201,9 @@
 	{
 		visibleGarrisonPoint.entity = entity;
 		cmpPosition.SetTurretParent(this.entity, visibleGarrisonPoint.offset);
+		let cmpUnitMotion = Engine.QueryInterface(entity, IID_UnitMotion);
+		if (cmpUnitMotion)
+			cmpUnitMotion.SetFacePointAfterMove(false);
 		let cmpUnitAI = Engine.QueryInterface(entity, IID_UnitAI);
 		if (cmpUnitAI)
 			cmpUnitAI.SetTurretStance();
@@ -298,6 +301,9 @@
 		if (vgp.entity != entity)
 			continue;
 		cmpNewPosition.SetTurretParent(INVALID_ENTITY, new Vector3D());
+		let cmpUnitMotion = Engine.QueryInterface(entity, IID_UnitMotion);
+		if (cmpUnitMotion)
+			cmpUnitMotion.SetFacePointAfterMove(true);
 		if (cmpUnitAI)
 			cmpUnitAI.ResetTurretStance();
 		vgp.entity = null;
Index: source/simulation2/components/CCmpPosition.cpp
===================================================================
--- source/simulation2/components/CCmpPosition.cpp	(revision 21631)
+++ source/simulation2/components/CCmpPosition.cpp	(working copy)
@@ -309,6 +309,7 @@
 
 	virtual void SetTurretParent(entity_id_t id, const CFixedVector3D& offset)
 	{
+		entity_angle_t angle = GetRotation().Y;
 		if (m_TurretParent != INVALID_ENTITY)
 		{
 			CmpPtr<ICmpPosition> cmpPosition(GetSimContext(), m_TurretParent);
@@ -325,6 +326,7 @@
 			if (cmpPosition)
 				cmpPosition->GetTurrets()->insert(GetEntityId());
 		}
+		SetYRotation(angle);
 		UpdateTurretPosition();
 	}
mimo added a comment.Mar 29 2018, 10:48 PM
In D1420#58215, @temple wrote:

I think it's better to set when setting the turrent parent, as you suggested earlier.

I've hesitated, but it was strange to set only the angle in OnEntityRenamed of GarrisonHolder, and all other properties set inside Promotion.

In D1420#58217, @temple wrote:

Also to stop the turning while garrisoning we should do SetFacePointAfterMove(false) from UnitMotion.

When is it needed for a turret?

In D1420#58218, @temple wrote:

I mean something like this:

Index: source/simulation2/components/CCmpPosition.cpp
===================================================================
--- source/simulation2/components/CCmpPosition.cpp	(revision 21631)
+++ source/simulation2/components/CCmpPosition.cpp	(working copy)
@@ -309,6 +309,7 @@
 	virtual void SetTurretParent(entity_id_t id, const CFixedVector3D& offset)
 	{
+		entity_angle_t angle = GetRotation().Y;
 		if (m_TurretParent != INVALID_ENTITY)
 		{
 			CmpPtr<ICmpPosition> cmpPosition(GetSimContext(), m_TurretParent);
@@ -325,6 +326,7 @@
 			if (cmpPosition)
 				cmpPosition->GetTurrets()->insert(GetEntityId());
 		}
+		SetYRotation(angle);
 		UpdateTurretPosition();
 	}

I've also envisaged that possibility, but that would change all the initial garrisoning angles (i mean all those not due to promotion). As no angle is given, they have a zero angle with respect to the turret (which looks fine for wall), with your proposition, they would always have angle=0 whatever the wall direction.

temple added a comment.EditedMar 29 2018, 11:14 PM
In D1420#58219, @mimo wrote:
In D1420#58217, @temple wrote:

Also to stop the turning while garrisoning we should do SetFacePointAfterMove(false) from UnitMotion.

When is it needed for a turret?

Right now the angle isn't set when they garrison, so they have the wrong angle as m_RotY. But they usually have a move command that ends with them turning to face the target, which in this case I think is the garrison holder from the point where they garrisoned. So they end up looking in kind of the right direction, but only after turning.

I've also envisaged that possibility, but that would change all the initial garrisoning angles (i mean all those not due to promotion). As no angle is given, they have a zero angle with respect to the turret (which looks fine for wall), with your proposition, they would always have angle=0 whatever the wall direction.

Right, I realized that shortly after. I'm guessing that should be an easy thing to add to TriggerHelper.

Unless they should always start at angle = wall angle after garrisoning, instead of the angle they came in at? Dunno.

mimo updated this revision to Diff 6293.Mar 31 2018, 10:56 AM

Rewritten (and extended) patch which now also allow to set the angle of turrets from the xml. There is still a problem with walls whose in and out directions is not clear and for them, we use the direction of the move to garrison as it is most often from inside to outward.

I think it's a bug that setting the turret position changes the angle silently (by changing the definition of m_RotY but not changing the value), so we should include that fix rather than doing some subtractions to account for it. (Of course then we'll have to do an addition in a different spot, but still.)

Some gates open inwards and some outwards, so I think we need to decide on a convention and enforce that in the art and the map wall functions. Buildings ungarrison at zero degrees, and we might prefer units to ungarrison inside our walls rather than outside, so maybe zero degrees should be facing inside? (Of course players can build their walls clockwise or counter-clockwise, but we can ensure that the premade ones go in the correct direction.)

binaries/data/mods/public/simulation/components/GarrisonHolder.js
82 ↗(On Diff #6293)

+points[point].Angle || undefined

219 ↗(On Diff #6293)

visibleGarrisonPoint.angle !== undefined

temple added inline comments.Mar 31 2018, 6:41 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
82 ↗(On Diff #6293)

I guess that's not exactly right, but the point is angle = 0 doesn't work with the if.

mimo updated this revision to Diff 6298.Apr 1 2018, 10:35 AM

updated patch

mimo added a comment.Apr 1 2018, 10:43 AM
In D1420#58272, @temple wrote:

I think it's a bug that setting the turret position changes the angle silently (by changing the definition of m_RotY but not changing the value), so we should include that fix rather than doing some subtractions to account for it. (Of course then we'll have to do an addition in a different spot, but still.)

Well, i would not call it a bug per se, but i fully agree that having the meaning of the rotation angle changing according to turret or not is really bugprone and should be changed. But that's outside the scope of that patch, and is more a post A23 feature than a bugfix.

In the patch, i've added some angles for the iber long wall for test purposes (not to be commited :)

binaries/data/mods/public/simulation/components/GarrisonHolder.js
82 ↗(On Diff #6293)

The problem is that points[point].Angle is "0", so if the + is outside the parenthesis it is evaluated as not false while inside as you propose it would be false and switch to undefined. But my approach had also a problem as +undefined evaluates as NaN.
Should be fixed in all cases now.

219 ↗(On Diff #6293)

yep

temple added a comment.Apr 1 2018, 6:25 PM
In D1420#58307, @mimo wrote:
In D1420#58272, @temple wrote:

I think it's a bug that setting the turret position changes the angle silently (by changing the definition of m_RotY but not changing the value), so we should include that fix rather than doing some subtractions to account for it. (Of course then we'll have to do an addition in a different spot, but still.)

Well, i would not call it a bug per se, but i fully agree that having the meaning of the rotation angle changing according to turret or not is really bugprone and should be changed. But that's outside the scope of that patch, and is more a post A23 feature than a bugfix.

I'm saying that it's expected that GetRotation().y before SetTurretParent is the same as GetRotation().y after. Setting a turret parent shouldn't turn the unit! Since we're here, we should fix that (it doesn't require any changes to m_RotY, just the two lines getting the rotation and setting it).

mimo updated this revision to Diff 6305.Apr 2 2018, 5:20 PM

update

Owners added a subscriber: Restricted Owners Package.Apr 2 2018, 5:20 PM
mimo added a comment.Apr 2 2018, 5:22 PM
In D1420#58313, @temple wrote:
In D1420#58307, @mimo wrote:
In D1420#58272, @temple wrote:

I think it's a bug that setting the turret position changes the angle silently (by changing the definition of m_RotY but not changing the value), so we should include that fix rather than doing some subtractions to account for it. (Of course then we'll have to do an addition in a different spot, but still.)

Well, i would not call it a bug per se, but i fully agree that having the meaning of the rotation angle changing according to turret or not is really bugprone and should be changed. But that's outside the scope of that patch, and is more a post A23 feature than a bugfix.

I'm saying that it's expected that GetRotation().y before SetTurretParent is the same as GetRotation().y after. Setting a turret parent shouldn't turn the unit! Since we're here, we should fix that (it doesn't require any changes to m_RotY, just the two lines getting the rotation and setting it).

OK i misunderstood what you asked for. Agree with that, and done in the patch.

temple added a comment.Apr 2 2018, 8:02 PM
In D1420#58362, @mimo wrote:

OK i misunderstood what you asked for. Agree with that, and done in the patch.

Yeah, about changing m_RotY after a23 I don't know. I guess we'd have to first create some moving turret parents, see if it makes more sense to have m_RotY relative to them or not. Maybe chariots for starters, that might not require any new art. Probably would want some changes to BuildingAI (some I've worked on). Anyway, that's for later.

binaries/data/mods/public/simulation/components/GarrisonHolder.js
49 ↗(On Diff #6305)

I don't think we can do Math.PI in the templates, so maybe should have the template angle in degrees then do the conversion in GarrisonHolder? Or not. Maybe have a help string explaining the clockwise convention anyway.

221 ↗(On Diff #6305)

I think that's probably the right choice, walls should face in and garrisoned units should face out. But anyway it can be decided later by whoever picks a convention for walls and gates and makes the art and map wall functions agree with that.

227 ↗(On Diff #6305)

Promotion sets the stance already so we probably shouldn't do this for those entities? But it doesn't set the previous stance there, so it's not working anyway. Meh.

binaries/data/mods/public/simulation/components/UnitAI.js
3387–3388 ↗(On Diff #6305)

I don't understand this. The previous way it effectively reset the default stance of turret units, so when they ungarrisoned they weren't in whatever stance the previous owner had them in. Here you're setting the turret stance of units already in turret stance, which means they'll be in standground when they ungarrison instead of the default aggressive.

mimo added inline comments.Apr 2 2018, 11:12 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
49 ↗(On Diff #6305)

I was also wondering if degrees would not be simpler to use, so let's go for it. Anyway, we'll see what's best only when we'll really start to use.

227 ↗(On Diff #6305)

right, copying previous stance could be added in promotion
and/or UnitAI could revert to default stance instead of keeping standground when no previous stance.
But that's for another patch.

binaries/data/mods/public/simulation/components/UnitAI.js
3387–3388 ↗(On Diff #6305)

I was too fast, and will remove that change

mimo updated this revision to Diff 6307.Apr 2 2018, 11:13 PM

updated patch

temple accepted this revision.Apr 3 2018, 2:22 AM
temple added inline comments.
binaries/data/mods/public/simulation/components/GarrisonHolder.js
85–87 ↗(On Diff #6307)

remove, the braces aren't necessary either

221 ↗(On Diff #6305)

On second thought, it's probably bad to hard-code a convention here. Maybe instead turrets should have a required angle rather than keeping their angle from before they garrisoned? But I guess that can be decided later, once we have more examples of turrets than units on walls. (For units on walls, it is nice to have the angle the same as before they garrisoned, since the walls might have been constructed backwards, with outside in and inside out.)

binaries/data/mods/public/simulation/templates/structures/iber_wall_long.xml
10 ↗(On Diff #6307)

remove

This revision is now accepted and ready to land.Apr 3 2018, 2:22 AM
mimo added inline comments.Apr 3 2018, 7:08 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
221 ↗(On Diff #6305)

It is used only for walls when garrisoned by triggerScripts: we don't want to have a required angle for them as keeping the garrisoning direction looks better, so we must provide a default when garrisoned from outWorld.
All other turrets (in mods as we only have walls currently) should provide an angle, although we cannot enforce it in the schema because of walls.

This revision was automatically updated to reflect the committed changes.