search

beyond-all-reason / spring

A powerful free cross-platform RTS game engine
https://beyond-all-reason.github.io/spring/
Other
182 stars 95 forks source link

Fix weapon aiming checks to use weapon's actual position and rotation. #1454

Closed marcushutchings closed 2 months ago

marcushutchings commented 2 months ago

Weapon rotating turrets with an aim constraint can find themselves unable to fire unless the front of the unit has rotated enough towards the weapon's target so that the starting direction of the weapon + the unit's front is within the tolerance. The issue can manifest with say armraz, where the guns are unable to fire (if a constraint is enforced see https://github.com/beyond-all-reason/spring/pull/1452) until the unit turns the legs around to face the target.

sprunk commented 2 months ago

Also it fails for weapons with trajectoryHeight.

marcushutchings commented 2 months ago

That behaviour is the whole point of the tag.

I don't understand what you are referring to. Please elaborate. I don't understand what tag was ever supposed to stop a constrained weapon from firing at something within its permitted firing arc because the unit's legs were facing the other way (when the weapon wasn't explicitly tagged as front facing only.)

CheckTargetAngleConstraint() does the front-facing restriction check if needed. That isn't affected. But if the foreward-only restriction isn't in place, then without this change the unit still doesn't fire if the weapon isn't facing towards the unit's front.

marcushutchings commented 2 months ago

Also it fails for weapons with trajectoryHeight.

A 2D arc check would solve that. Though, will there be any issues with units being able to shoot near 90 degrees up down? If not then the adjustment is simple. If there isn't then I'll need to add some extra checks.

sprunk commented 2 months ago

I don't understand what tag was ever supposed to stop a constrained weapon from firing at something within its permitted firing arc because the unit's legs were facing the other way (when the weapon wasn't explicitly tagged as front facing only.)

maxAngleDif controls the arc in relation to mainDir. mainDir (and thus the arc) is relative to the unit (i.e. "legs"), and within unit space it is static (so the only way to point it in a different world-space direction is to rotate the whole unit, i.e. "make legs face the correct way").

CheckTargetAngleConstraint() does the front-facing restriction check if needed. That isn't affected.

I tested and it was affected. I didn't test current head so hopefully it's this patch and not something already merged recently. I'm not at home at the moment, i'll send some screenshots in about 10 hours. Generally get a ZK gunshipkrow and target ground randomly around the unit until you get all 3 guns firing at the same spot. This shouldn't be possible because the arcs are set up such that at most 2 overlap.

sprunk commented 2 months ago

The use case for firing arcs is "battleship" style turrets. They are not meant to aim backwards, the whole body of the unit is supposed to rotate to put targets in the turret's firing arc. image

marcushutchings commented 2 months ago

What you are explaining there means there is no way to have a turret with a 90 degree firing arc ever fire behind the unit. I think we're talking about two different cases here.

sprunk commented 2 months ago

Set mainDir = [[0 0 -1]]?

marcushutchings commented 2 months ago

I don't understand what tag was ever supposed to stop a constrained weapon from firing at something within its permitted firing arc because the unit's legs were facing the other way (when the weapon wasn't explicitly tagged as front facing only.)

maxAngleDif controls the arc in relation to mainDir. mainDir (and thus the arc) is relative to the unit (i.e. "legs"), and within unit space it is static (so the only way to point it in a different world-space direction is to rotate the whole unit, i.e. "make legs face the correct way").

CheckTargetAngleConstraint() does the front-facing restriction check if needed. That isn't affected.

I tested and it was affected. I didn't test current head so hopefully it's this patch and not something already merged recently. I'm not at home at the moment, i'll send some screenshots in about 10 hours. Generally get a ZK gunshipkrow and target ground randomly around the unit until you get all 3 guns firing at the same spot. This shouldn't be possible because the arcs are set up such that at most 2 overlap.

Did you test a weapon with onlyForward flag set?

sprunk commented 2 months ago

Not yet, I'll be back home in 10h and test more.

marcushutchings commented 2 months ago

Set mainDir = [[0 0 -1]]?

We're talking about different things here. Okay. You're talking about a fix placement turret. Okay. I see where my change would impact the original intent.

However, we have another issue, which looks to be a new case. A Razorback can rotate its body, separate to the legs. From what we already have the turret technically can fire in any direction --- however, this is a problem, because the unit can shoot out the back of the gun to hit an enemy behind it. We can't use the current restrictions because the turret could go outside the aim angle and then wouldn't be able to fire (which looks bad), but it would still be able to shoot through itself if the legs are facing the target.

Okay. I'll fix this up. Thanks for your patience.

sprunk commented 2 months ago

You're talking about a fix placement turret. Okay. I see where my change would impact the original intent.

Yes, though I think onlyForward is supposed to behave the same as mainDir = [[0 0 1]] as far as aiming/targeting is concerned. I am not sure what the full list of differences is, there's a ticket to discover it #1449

the turret technically can fire in any direction --- however, this is a problem, because the unit can shoot out the back of the gun to hit an enemy behind it.

There's maxFireAngle which I believe is meant to do what you want (ZK seems to set it to 360° for most units so I'm not that familiar with it). AFAIK right now it only works for initiating the burst though. https://github.com/beyond-all-reason/spring/blob/6cefa051663284da0043c12075f11da45808dfda/rts/Sim/Weapons/WeaponDef.h#L96

I think it would be fine to extend it to also work on the bursts. A minor issue to keep in mind when checking it out: #1450

sprunk commented 2 months ago

Somewhat relatedly: it would be good if there was a debug mode that showed weapon vectors, each in unique color:

AFAIK the former two may be shown in /debugcolvol already but they are directionless balls showing just the piece origin and not rotation.

marcushutchings commented 2 months ago

maxFir

Somewhat relatedly: it would be good if there was a debug mode that showed weapon vectors, each in unique color:

* direction of the piece returned from QueryPiece

* direction of the piece returned from AimFromPiece

* direction of the `weaponDir` because it seems used a lot but not identical with any of the others

* cone of firing arc

AFAIK the former two may be shown in /debugcolvol already but they are directionless balls showing just the piece origin and not rotation.

Sounds like a good ticket,

marcushutchings commented 2 months ago

You're talking about a fix placement turret. Okay. I see where my change would impact the original intent.

Yes, though I think onlyForward is supposed to behave the same as mainDir = [[0 0 1]] as far as aiming/targeting is concerned. I am not sure what the full list of differences is, there's a ticket to discover it #1449

the turret technically can fire in any direction --- however, this is a problem, because the unit can shoot out the back of the gun to hit an enemy behind it.

There's maxFireAngle which I believe is meant to do what you want (ZK seems to set it to 360° for most units so I'm not that familiar with it). AFAIK right now it only works for initiating the burst though.

https://github.com/beyond-all-reason/spring/blob/6cefa051663284da0043c12075f11da45808dfda/rts/Sim/Weapons/WeaponDef.h#L96

I think it would be fine to extend it to also work on the bursts. A minor issue to keep in mind when checking it out: #1450

Thanks. I'm trying it out, but it doens't seem to be working. Though I can't quite get my head around how these angles work for this purpose. It doesn't look like it matches the weapon model angles. I'll play around with it some more.

marcushutchings commented 2 months ago

It doesn't seem to work for razorback, it may be an issue with the aiming scripts or some assumptions.

This is a check for a target that out of the 20 degree arc - it is nearly 90 degress. The aim check code for maxFireAngle says the angle is near perfect = 0.9999999 wantedDir.dot(lastRequestedDir) <= weaponDef->maxFireAngle UpdateSalvo: (0.420028,-0.294809,0.858291).dot(0.420034,-0.294844,0.858276) = 0.999999 <= 0.939722

This is the same check but using the weapons aim position and real direction. This would prevent the shot. weaponDir.dot((aimFromPos - currentTargetPos).SafeNormalize2D()) < weaponDef->maxFireAngle UpdateSalvo: (0.872078,-0.186148,-0.452571).dot(-0.439564,0.000000,-0.898211) = 0.023170 <= 0.939722

In the second one, I safeNormalize2D the target direction, which is why the y = 0.0. I probably should have done the same for the weaponDir, but it is still good enough to illustrate the issue. The shot looks very obviously wrong if controlled by the first example.

sprunk commented 2 months ago

I believe the model is irrelevant and the weapon dir is set immediately. The reason the unit doesn't then immediately shoot is that AimWeapon needs to return true (I think in engine the return value from AimWeapon is called angleGood).

IIRC the case that prompted fireTolerance is this: 1) get a unit (say a turret so it doesn't rotate the body). 2) Target something. AimWeapon is called, which usually means the model rotates the turret piece and calls WaitForTurn, but I think any delay would do. 3) predict when AimWeapon is about to finish its delay. If the delay is via animation then it's easy to tell. Make it so that 1 simframe before the delay is over, the target is somewhere else (for example completely opposite). This can be via target change (easier to reproduce), or because the target was a unit who just moved (harder to reproduce manually but still important). You don't need to be frame-precise but keep in mind AimWeapon is periodically called again and you need to do it while the current one, for the previous direction, is still ongoing. 4a) AimWeapon returns true. This means the weapon is aimed correctly and we are about to fire. 4b) fireTolerance (aka maxFireAngle in engine internals) compares the angle the AimWeapon was called with and the current angle and gets an opportunity to say "hol' up motherfucker". 4c) if fire tolerance doesn't complain, we fire (possibly backwards).

.5) if the weapon is a burst, you can keep switching targets and it will shoot accordingly. fireTolerance doesn't get a say anymore at this step.

I think to reproduce the scenario you would just need to both set fireTolerance to 360' in a Lua weapondef and also remove the hardcoded 20' cap mentioned in #1450, so that step 4b doesn't interrupt it.

image

Some relevant caveats:

sprunk commented 2 months ago

Razorback has a burst so the current implementation of firetolerance won't work for it.

marcushutchings commented 2 months ago

The question is, how do we achieve this for burst? Direct line, aiming is easy enough, but for weapons that want to aim upwards, that's more involved. We may have to force an AimWeapon script call when the target changes?

sprunk commented 2 months ago

The question is, how do we achieve this for burst?

I don't know, I'm just saying that I believe firetolerance is the correct tag to use here and that I believe it works for non-burst (burst=1) weapons.

Direct line, aiming is easy enough, but for weapons that want to aim upwards, that's more involved.

I don't think this matters. Ballistic weapons still fundamentally aim in a direct line at some direction in the sky, just the direction is carefully picked so that the projectile manages to fall down onto the target.

We may have to force an AimWeapon script call when the target changes?

This would be good but doesn't solve all cases. The target can be a unit which can then just move to the other side of the shooter. The best example is a teleport ability but even without Lua you can have very fast aircraft fly overhead. The target's position changes but its identity doesn't.

marcushutchings commented 2 months ago

I see. yes, I think firetolerance sounds like the right parameter.

marcushutchings commented 2 months ago

Closing. This isn't the way forward. These changes interferre with existing behaviours that are needed. https://github.com/beyond-all-reason/spring/pull/1452 is probably better.