FAForever / fa

Lua code for FAF
226 stars 232 forks source link

Fix UEF SACU fire rate upgrade using inaccurate fire rate value #6121

Closed lL1l1 closed 4 months ago

lL1l1 commented 5 months ago

Description of the proposed changes

Changes the UEF SACU's fire rate upgrade from 1.82 fire rate to 2.00 fire rate because 1/1.82 = 0.5494 so it rounds down to 0.5 fire delay, which is equal to 2 fire rate. Also updates the localizations.

Testing done on the proposed changes

Spawn a UEF combatant preset and step 5 ticks while paused to confirm that it does shoot 2 times per second with the 1.82x fire rate.

Checklist

Garanas commented 5 months ago

We recently introduced a test suite for invalid fire rates, see also;

https://github.com/FAForever/fa/blob/f7420f632083217061e651e0b7097f345e6d2e29/tests/blueprint/unit.spec.lua#L278-L315

Would you be interested to enhance the test suite to catch these cases too?

lL1l1 commented 5 months ago

This isn't a balance change, its effect is only for the UI.

lL1l1 commented 5 months ago

There's only 4 units with fire rate enhancements, does it need a test?

Garanas commented 5 months ago

oh, I miss understood then. I thought this was a more broader problem! Great solution however 😃

MrRowey commented 5 months ago

This isn't a balance change, its effect is only for the UI.

you have changed a Unit Fire Rate tho so that is a balance Change

Garanas commented 5 months ago

This isn't a balance change, its effect is only for the UI.

you have changed a Unit Fire Rate tho so that is a balance Change

If I'm not mistaken, practically it is the same value due to rounding 😃 ! It's now just more obvious what the value is.

Garanas commented 5 months ago

It appears the tests do not run on this branch 🤔 , any idea as to why that is?

lL1l1 commented 5 months ago

Not sure why it doesn't show up in the "checks" tab of the PR, but the tests do run on my fork's page https://github.com/lL1l1/fa/actions/workflows/test.yaml

MrRowey commented 5 months ago

That's gin then feel free to merge then when you happy jip

MrRowey commented 4 months ago

@lL1l1 is this ready for review

lL1l1 commented 4 months ago

yes