Phobos-developers / Phobos

Ares-compatible C&C Red Alert 2: Yuri's Revenge engine extension
GNU Lesser General Public License v3.0
300 stars 95 forks source link

Infantry firing while moving #1341

Open TaranDahl opened 3 months ago

TaranDahl commented 3 months ago

Infantry firing while moving

In rulesmd.ini:

[SOMETECHNO]                 ; InfantryType
FiringByPassMovingCheck=true ; boolean
github-actions[bot] commented 3 months ago

Nightly build for this pull request:

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.

mevitar commented 3 months ago

It's actually possible to have infantry firing on move in unmodified YR, it just needs to have its firing sequence set to something like 0,1,1 and thus making its firing sequence not play at all.

I did tests with this build, and it seems that for ground infantry, there is no difference between having a "clear" firing sequence or adding FiringByPassMovingCheck=yes. The firing sequence does not play on move in either case. However, FiringByPassMovingCheck=yes does allow the infantry to fire on the move, and still have and use a regular firing sequence while stationary.

The biggest difference is for jumpjet infantry. Using FiringByPassMovingCheck=yes and JumpJetTurn=no like stated in the description makes the infantry able to fire on the fly and still face the target as its attack sequence plays. Didn't notice any issues here, but i did no multiplayer testing.

Metadorius commented 3 months ago

It could be cool to add a new sequence of firing on the move, but it would probably be hard to meaningfully do so because in the ideal case you would need up to 8 movement directions x 8 firing directions = 64 sequences, I think.

mevitar commented 3 months ago

It's not just 64 sequences, it's 64 sequences for fire-move on ground, 64 sequences for fire-move on water, and 64 for fire-move in air.

Starkku commented 3 months ago

Don't forget that secondary weapon slot can have its own firing sequence on land. Not to mention both it and primary weapon have crawling firing sequences available.

TaranDahl commented 3 months ago

True enough. At present this pr is mainly serving jumpjets. Additional sequences are needed, but I think it is hard to impl in a short time.

TaranDahl commented 3 months ago

again the tag name is not very clear. Suggest to change FiringByPassMovingCheck to something like OpportunityFire.AllowInfantry to indicate it's only for infantry and requires previous OpportunityFire to work

also suggest to change the hook names from InfantryClass_CanFire_HitAndRun to InfantryClass_CanFire_OnMoving for better clarity

I think the flag name should not mention OpportunityFire directly, considering the real behavior. At first, I bound it to OpportunityFire=yes, but later discovered that it could run independently without OpportunityFire. Using the "ctrl+shift" command on infantries with this flag will let them fire while moving, just like you can use that command to make Apocalypses fire while moving, though they have no OpportunityFire=yes set.

TaranDahl commented 3 months ago

again the tag name is not very clear. Suggest to change FiringByPassMovingCheck to something like OpportunityFire.AllowInfantry to indicate it's only for infantry and requires previous OpportunityFire to work

also suggest to change the hook names from InfantryClass_CanFire_HitAndRun to InfantryClass_CanFire_OnMoving for better clarity

The problems with the hook name and code style are fixed.

mevitar commented 3 months ago

True enough. At present this pr is mainly serving jumpjets. Additional sequences are needed, but I think it is hard to impl in a short time.

If any new sequences are added, i think just a single FireMove= is sufficient. Making separate sequence for each direction is too complicated and i think not worth the effort (how many people will be making use of it anyway?). Not to mention that, if infantry is expanded, then people will soon after start demanding the same be done for vehicles, and if that is not a feature creep, i don't know what is.

So i think that adding new sequences is out of scope for this feature anyway. Let's keep it simple, and worry about new sequences in the future.

Otamaa commented 3 months ago

True enough. At present this pr is mainly serving jumpjets. Additional sequences are needed, but I think it is hard to impl in a short time.

Implementing additional infantry sequences is not that difficult , I add 2 additional infantry sequences myself , it is just annoying to hook since it called on few places .