Support broadside attack facing#933
Conversation
b43bbb4 to
110a22a
Compare
|
Thanks, looks pretty good. A few issues, I've submitted a patch on your branch here: richardspink2#1
|
Imported the Civ3 TurnToAttack flag and use it to rotate attack animations counter-clockwise for broadside units while keeping combat calculations based on the real attack direction. Added rotateBeforeAttack data to the base ruleset so new games load the same unit prototype behavior. Manual test: created a loadable save file with two Frigates on adjacent coast tiles, loaded it in OpenCiv3, and fought the ships to check the firing direction. Validation: dotnet build C7/C7.sln; dotnet format C7/C7.sln whitespace --verify-no-changes; dotnet test C7/C7.sln with CIV3_HOME set; git diff --check; dotnet list C7/C7.sln package --vulnerable --include-transitive.
110a22a to
b952f21
Compare
|
Thanks for catching that. I did test it with a loadable save containing two adjacent Frigates, but I was checking the broadside behavior rather than the handedness of the rotation. I’ve corrected it to CCW, added the base-ruleset entries, rebased on latest Development, and added automated coverage so this no longer depends on visual inspection. |
stavrosfa
left a comment
There was a problem hiding this comment.
Thanks, looks good overall!
Declare rotateBeforeAttack for every base ruleset unit, including false values for non-rotating units. Use explicit attacker and defender combat direction names for readability. Tested firing direction with a loadable save by fighting two adjacent ships (Frigates). Validation: dotnet build C7/C7.sln; dotnet format C7/C7.sln whitespace --verify-no-changes; dotnet test C7/C7.sln with CIV3_HOME set; git diff --check; dotnet list C7/C7.sln package --vulnerable --include-transitive.
|
Don't hate me for this @richardspink2, usually I am not that slow, but I totally missed this up until now. This is the first, (if I am not completely mistaken) of what the editor calls Unit Abilities, to be implemented.
I think the best way to go about this, is to have these as flags, similar to what buildings do e.x.
For 1 such ability, having boolean fields is not such a big deal, but there are 31 abilities * 124 unit prototypes = 3844 fields, with mostly grabage information. I don't know if that makes sense to you both. |
Store RotateBeforeAttack as a unit prototype flag instead of a saved boolean field. Keep rotateBeforeAttack as a runtime convenience property backed by the flag, and import Civ3 TurnToAttack into the flag set. Update the base ruleset so only the TurnToAttack units declare flags: [rotateBeforeAttack], avoiding false entries for every other unit prototype. Tested firing direction with a loadable save by fighting two adjacent ships (Frigates). Validation: dotnet build C7/C7.sln; dotnet format C7/C7.sln whitespace --verify-no-changes; dotnet test C7/C7.sln with CIV3_HOME set; git diff --check; dotnet list C7/C7.sln package --vulnerable --include-transitive.
|
That makes sense. I updated this to use a unit prototype flag instead of a saved boolean field, matching the existing building/tech flag pattern. The runtime still has a rotateBeforeAttack convenience property backed by the flag, and the base ruleset now only marks the TurnToAttack units with flags: ["rotateBeforeAttack"] instead of carrying false values for every other unit. |
|
LGTM, working fine. |
stavrosfa
left a comment
There was a problem hiding this comment.
Thanks, looking great overall!



Summary
TurnToAttackflag into unit prototype data.rotateBeforeAttackdata to the base ruleset so new games load the same unit prototype behavior.Fixes #902.
Testing
dotnet build C7/C7.slndotnet format C7/C7.sln whitespace --verify-no-changesdotnet test C7/C7.slnwithCIV3_HOMEset: 62 passedgit diff --checkdotnet list C7/C7.sln package --vulnerable --include-transitive: no vulnerable packages