Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and robustify transport cargo killing and veterancy #6091

Merged
merged 32 commits into from Apr 22, 2024

Conversation

clyfordv
Copy link
Contributor

  1. When killing cargo, the instigator unit is provided. The engine then counts kills for score and unitview purposes.
  2. Similarly, the mass of all cargo is distributed to the killer(s) (via lua) with VeterancyComponent.VeterancyDispersal.
  3. For transports with external storage, cargo is cached so impact effects can be delayed/applied properly when the transport hits the ground.
  4. Lastly, the new function explicitly kills all units (previously relied on the engine to do it), which (hopefully) addresses a bug where units who were supposed to die escaped their fate.

And hopefully all in a much more straightforward way than before!

Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the changes with continentals of SACU or harbingers, a T1 transport with SACU/harbingers, a carrier full of strategic bombers, and a CZAR. The veterancy distribution works.
In the suggestions, I fixed two bugs I found:

  • units don't detach from air transports nor spawn wrecks on transport death. They also play death effects when the transport dies.
  • Units in carriers (transport internal storage) play death effects/weapons (but aircraft don't have death weapons so its less noticeable). They shouldn't do this because it looks bad.

There is another bug that I'm unsure of the best way to fix:

  • transports/carriers can't be ctrl-k'd.
    This bug is caused because Entity:Kill() is separate from Entity:Kill(instigator, damageType, excessDamageRatio), and the latter is called if any 3 arguments are passed to :Kill, even if they are nil, and the arguments must be valid (instigator is Unit | nil, damageType is string, excessDamageRatio is number).
    Solutions: Add (nil, '', 0) to selfdestruct.lua:45, add if not instigator then damageType = '' excessDamageRatio = 0 end to the AirTransportUnit and AircraftCarrierUnit Kill functions, or use BaseUnitKill(self) when instigator == nil inside the unit classes and inside killcargo.

The best fix would just be whatever is most easily documented I guess.

Future? possible improvements:

  • wrecks from transports' internal storages
  • can an internal storage + external clamps transport exist? If so, would it break the logic?

Don't forget the changelog by the way.

lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/AircraftCarrierUnit.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
@lL1l1 lL1l1 added area: sim Area that is affected by the Simulation of the Game feature: transports related to the behavior of transports area: balance related to units balance labels Apr 17, 2024
@lL1l1 lL1l1 linked an issue Apr 17, 2024 that may be closed by this pull request
clyfordv and others added 7 commits April 17, 2024 00:10
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
@clyfordv
Copy link
Contributor Author

Fixed ctrl-k, probably had something to do with removing the part with comments to the effect of "do not remove this part or it won't work".

@MrRowey MrRowey added the DO NOT MERGE Don't Merge until removed label Apr 17, 2024
@MrRowey
Copy link
Member

MrRowey commented Apr 17, 2024

Please Request a review from me once PR is Complete :)

@clyfordv
Copy link
Contributor Author

Alright we should be solid (tested it this time!)

Improvements:

  • Added TransportInternal and TransportExternal as damage types (currently only TransportInternal is relevant, so we could trim it down to just Transport if people like the simplicity).
  • Reworked the logic so KillCargo only runs once on transports-in-transports.
  • OnKilled for mobile units and aircraft runs a check for the TransportInternal damage type and will not play any effects, death threads, shield bounce projectiles, etc. when the internal storage they're in dies.

Possible issues:

  • The divide between score/AI and visual/audio/etc. in OnKilled is not super strong, or at least not formalized. (For example, do death threads do anything that should still happen if killed inside a transport?)

Minor issues:

  • Discovered that external factory dummy units are counted as losses to the player at the end of the game (2, actually--1 structure, 1 layer). This happens in deploy/fafdevelop as well.

Future ideas:

  • Tangentially related, but when a transport enters a transport it could detach its unit, store it in the carrier on another bone, then retrieve it when it exits. This would require a little rework of the above, but would solve the ghost selection issue when a stinger with cargo is in a carrier.

@clyfordv clyfordv requested a review from MrRowey April 17, 2024 19:28
@MrRowey
Copy link
Member

MrRowey commented Apr 17, 2024

@relent0r will any of the changes here affect ai ?

@clyfordv
Copy link
Contributor Author

I mediated on it for a bit, trimming down to a single damage type ("TransportDamage") felt right. Let me know if there's any objections.

Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and found no errors.

lua/sim/Unit.lua Outdated Show resolved Hide resolved
lua/sim/units/AirTransportUnit.lua Outdated Show resolved Hide resolved
lua/sim/units/AircraftCarrierUnit.lua Outdated Show resolved Hide resolved
lua/sim/units/MobileUnit.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/sim/units/components/TransportUnitComponent.lua Outdated Show resolved Hide resolved
lua/armordefinition.lua Outdated Show resolved Hide resolved
clyfordv and others added 7 commits April 18, 2024 09:26
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>
@relent0r
Copy link
Contributor

@relent0r will any of the changes here affect ai ?

I don't see anything that would impact the default AI. The changing from OnKilled to Kill might impact other AI's that are tracking deaths of units. But it looks like we are trying to make it consistent between files so makes sense.

@MrRowey
Copy link
Member

MrRowey commented Apr 21, 2024

@lL1l1 you happy with the changes so far? if so can you approve the PR so I know

@clyfordv
Copy link
Contributor Author

killedInTransport flagging variable is renamed back, so we should be squared away

@MrRowey MrRowey merged commit 5f7b2a9 into FAForever:deploy/fafdevelop Apr 22, 2024
clyfordv added a commit to clyfordv/fa that referenced this pull request Apr 23, 2024
commit ceb09ce
Author: Josh <row.josh02@gmail.com>
Date:   Tue Apr 23 09:35:48 2024 +0100

    Balance : TML Hp Reduction & Adds Death Weapon (FAForever#6104)

    Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>

commit 23ebe48
Author: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Date:   Mon Apr 22 12:06:13 2024 -0700

    Fix units not rebuilding on army transfer (FAForever#6106)

commit 5f7b2a9
Author: clyf <clyfordv@gmail.com>
Date:   Mon Apr 22 04:15:20 2024 -0400

    Simplify and robustify transport cargo killing and veterancy (FAForever#6091)

    Co-authored-by: lL1l1 <82986251+lL1l1@users.noreply.github.com>

commit d3ef3c0
Author: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Date:   Sun Apr 21 10:02:18 2024 -0700

    Inherit all damage data in Ahwassa bomb script (FAForever#6102)

commit f925f48
Author: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Date:   Sat Apr 20 22:21:57 2024 -0700

    Refactor collision beams (FAForever#6065)

commit 24abffa
Author: G C <37224614+Hdt80bro@users.noreply.github.com>
Date:   Sat Apr 20 08:45:44 2024 -0700

    Quicken inevitable voting results (FAForever#5088)

commit 2547d18
Author: (Jip) Willem Wijnia <w.b.wijnia@gmail.com>
Date:   Sat Apr 20 09:28:24 2024 +0200

    Introduce basic anti-spam protection from taunts (FAForever#6099)

commit 1635090
Author: (Jip) Willem Wijnia <w.b.wijnia@gmail.com>
Date:   Sat Apr 20 09:25:23 2024 +0200

    Revert "Remove mass from dead trees and tree groups" (FAForever#6101)

commit ca94bd8
Author: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Date:   Fri Apr 19 15:09:17 2024 -0700

    Revert "Remove mass from dead trees and tree groups" (FAForever#6100)

commit 524fc7f
Author: clyf <clyfordv@gmail.com>
Date:   Fri Apr 19 10:42:27 2024 -0400

    Fix being able to detach the external factory of the primary unit (FAForever#6093)

commit 7993337
Author: lL1l1 <82986251+lL1l1@users.noreply.github.com>
Date:   Thu Apr 18 00:53:13 2024 -0700

    Revert hoplite firing from transports (FAForever#6094)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: balance related to units balance area: sim Area that is affected by the Simulation of the Game DO NOT MERGE Don't Merge until removed feature: transports related to the behavior of transports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Units in transports are not counted as kills
4 participants