OpenTopped moving fire customization

[Coronia]

• Whether or not a passenger's weapon can fire out from an OpenTopped=yes transport will now respect the weapon's FireWhileMoving setting.
• OpenTopped/OpenTransport.FireWhileMoving can be used to customize whether or not passengers can fire out when the transport is moving, for transport and passenger respectively. Both of them and the weapon's FireWhileMoving toggle need to be set to true to allow firing out when moving.

In rulesmd.ini:

[General]
OpenTopped.FireWhileMoving=true                   ; boolean
OpenTransport.FireWhileMoving=true                ; boolean

[SOMETECHNO]                                      ; TechnoType, transport with OpenTopped=yes
OpenTopped.FireWhileMoving=                       ; boolean, defaults to [General] -> OpenTopped.FireWhileMoving

[SOMETECHNO]                                      ; TechnoType, passenger
OpenTransport.FireWhileMoving=                    ; boolean, defaults to [General] -> OpenTransport.FireWhileMoving

[github-actions]

Nightly build for this pull request:

• compiled-dll-b271c01fb6d55ec1e13341b76ff03a418c47cc4f.zip
  These artifacts will expire in 90 days and will not be available for download after that time.

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

[Starkku]

This technically works little bit different from actual OpportunityFire which contrary to its name actually affects target scanning not firing. Whether or not the distinction is important I am uncertain about, though.

It might be more sensible to use name like AllowFireWhileMoving or whatever since that one explicitly affects firing and is more similar to this in concept. Vanilla code FireWhileMoving also returns FireError::MOVING instead of FireError::ILLEGAL like you are currently doing but I am unsure if the OpenTopped context changes requirements here.

[TaranDahl]

1. It doesn't have any special handling anywhere - only affects behaviour by not being any of the other enum values, primarily FireError::OK, so returning it behaves mostly same as FireError::Illegal with exception of latter having target reset for repair weapons.

I believe the main difference is that Illegal will clear the target. What it seems to intend to convey is "invalid target", meaning "It’s not my fault that firing is unavailable; the problem lies with that target, so forget about it."

2. potentially bypass several other checks

What is it specifically?

[Starkku]

The fire error code should maybe be changed back to Illegal from Moving after all. The order of the checks in vanilla game is somewhat deliberate and while fully invalidating (Illegal) firing at any point is completely fine, returning a non-illegal fire status bypasses all checks that could later invalidate it.

(Could also argue from the angle of semantics with this being techno-level evaluation and movement being a property of FootClass and derivatives, so if we're nitpicking these checks should exist elsewhere but I get the appeal of leaving them all in one place.)

Because every other code than OK prevents firing the visible effects of this are likely to be minor or non-existent and would likely require testing to verify such edge cases are not problematic. Mostly I'd check target retention and if repair weapons inside transports behave correctly. It is possible no change is required if everything behaves as expected.

---

I believe the main difference is that Illegal will clear the target. What it seems to intend to convey is "invalid target", meaning "It’s not my fault that firing is unavailable; the problem lies with that target, so forget about it."

That seems to be the semantic meaning yes.

potentially bypass several other checks

Mostly the aforementioned repair weapon stuff but it is possible it doesn't matter as it will be evaluated correctly the moment the unit stops moving and weapon would potentially be allowed to fire again anyway.

Sorry I removed my original comment as it had some erroneous assumptions and I had hoped nobody had time to reply to it before I could do so.

---

TL;DR: If FireError::Moving works correctly for target retention, repair weapons etc. just leave it alone after all.

[Coronia]

The current implementation doesn't make its FireError code change from Illegal to Moving either, cuz it'll still be overwritten at a later point. If we decide to use Moving, then it probably requires another hook to bypass that point

[TaranDahl]

Alright. I have no objectation with Starkku's judgement.

[TaranDahl]

You may add some comments in the doc.
"Because of technical issues, this feature's behavior is somewhat different from vanilla FireWhileMoving. This may be changed in the future."