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.
Details
Details
Try to garrison sone units on wall, and promote them.
Diff Detail
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
Comment Actions
I think it's better to set when setting the turrent parent, as you suggested earlier.
Comment Actions
Also to stop the turning while garrisoning we should do SetFacePointAfterMove(false) from UnitMotion.
Comment Actions
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(); }
Comment Actions
I've hesitated, but it was strange to set only the angle in OnEntityRenamed of GarrisonHolder, and all other properties set inside Promotion.
When is it needed for a turret?
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 |
| 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. |
Comment Actions
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. |
| 219 ↗ | (On Diff #6293) | yep |
Comment Actions
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).
Comment Actions
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. |
| 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 |
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
| 3387–3388 ↗ | (On Diff #6305) | I was too fast, and will remove that change |
| 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 |
| 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. |