Opened 8 years ago
Last modified 17 months ago
#3866 new defect
[PATCH] When a ship sinks/building destroys without space to unload units aren't lost in summary
Reported by: | eternaf | Owned by: | Mercury |
---|---|---|---|
Priority: | Must Have | Milestone: | Backlog |
Component: | Simulation | Keywords: | patch, simple |
Cc: | Imarok | Patch: | Phab:D4643 |
Description
If you sink a ship, any unit garrisoned on it and the ship itself won't get counted as lost in the summary of the game.
Attachments (1)
Change History (20)
by , 8 years ago
Attachment: | garrison_depth.js.patch added |
---|
comment:2 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 21 |
Owner: | set to |
Status: | new → assigned |
Summary: | When a ship sinks, units aren't lost in summary → [PATCH] When a ship sinks/building destroys without space to unload units aren't lost in summary |
follow-up: 4 comment:3 by , 8 years ago
The logic may or maynot be moved from GarrisonHolder
or StatisticsTracker
, not sure what's worse.
What if you garrison a garrisoned ram onto a ship which sinks? Are there actually further bugs than not counting those units with the statistics tracker, as those entities are not added to killedEntities? Could you do some philadelphia experiments to find out?
comment:4 by , 8 years ago
Replying to elexis:
What if you garrison a garrisoned ram onto a ship which sinks? Are there actually further bugs than not counting those units with the statistics tracker, as those entities are not added to killedEntities? Could you do some philadelphia experiments to find out?
What's the ram? I could try to garrison another types of units. Logic of using StatisticsTracker is present in few other components.
follow-up: 6 comment:5 by , 8 years ago
Ram as in https://en.wikipedia.org/wiki/Battering_ram, as said above, one can garrison infantry onto a ram and garrison the ram onto a ship. Will the staticstracker be updated with these recursively garrisoned units too? Will cmpHealth.Kill()
be called for those?
comment:6 by , 8 years ago
Replying to elexis:
Ram as in https://en.wikipedia.org/wiki/Battering_ram, as said above, one can garrison infantry onto a ram and garrison the ram onto a ship. Will the staticstracker be updated with these recursively garrisoned units too? Will
cmpHealth.Kill()
be called for those?
It works for this case too. It will be recursively updated.
comment:7 by , 8 years ago
Keywords: | review removed |
---|
So far so good, but
- If a player deletes units, they won't count as "lost", but with your patch they would in case theywere garrisoned on a deleted ship. After reading
Damage.TargetKilled
it becomes clear that the stat should reflect how many units were killed by enemies cmpKillerPlayerStatisticsTracker.KilledEntity
should therefore be called too- You could try to call
GarrisonHolder.EjectOrKill
fromDamage.TargetKilled
with an additional argument giving the killer (and if the killer isn't set, it was likely deleted). - Another edge case would be garrisoning units on an allied ship and then changing the diplomacy in the middle of the ocean. I guess in this case it should also count as actively killed units (killed by the ship).
comment:8 by , 8 years ago
Keywords: | simple added |
---|---|
Milestone: | Alpha 21 → Backlog |
Backlogging due to lack of progress.
comment:9 by , 7 years ago
Cc: | added |
---|
comment:10 by , 7 years ago
Owner: | removed |
---|---|
Priority: | Should Have → Must Have |
Status: | assigned → new |
If a player deletes something (Commands.js
), the GarrisonHolder entities can be deleted manually (has to be done recursively since there can be ram garrisoned with units garrisoned in the ship).
The case of changing diplo in the middle of the sea can likely be ignored.
Also there are other cases where Kill
is called but not the statistics tracker, for example extinct volcano, the Cheat or UnitMotionFlying.
comment:11 by , 6 years ago
Damage.TargetKilled
is called after the target has already been killed. So the GarrisonHolder.OnHealthChange
will be called earlier and therefore EjectOrKill
,right?
Moving with a similar patch, it will be really hard to tell who killed the units garrisoned.
For alternatives, don't you think that the Health.kill
should differentiate between an entity being deleted and being killed -for example Health.delete
? Furthermore, should the Health component or atleast the OnHealthChange
know what caused this change and who -take different actions according to who hurt or healed me, right?
Considering that the Health component can know what and who caused the change, this can be easily solved.
comment:12 by , 6 years ago
Statistics for killing and lost garrisoned units should work the same way like stats for ungarrisoned units.
Hm, EjectOrKill can also be called on ownershipchange and diplomacychange. I guess these messages would need extension too if we don't want to close this as won't fix. Should check that there isn't a performance problem as at least the healthchanged one can be sent often. But probably isn't.
comment:13 by , 5 years ago
Component: | UI & Simulation → Simulation |
---|
Move tickets to Simulation
as UI & Simulation
got some sub components.
comment:14 by , 3 years ago
Keywords: | simple removed |
---|---|
severity: | → simple |
comment:15 by , 3 years ago
Keywords: | simple added |
---|
comment:16 by , 2 years ago
I made a fix for this. Addresses garrisonable entities being ejected due to damage, ownership change, diplomacy change or garisonable type change. calls KilledBy from EjectOrKill. Creates a new event named DamageTaken which fires after damage is done but before checking for death and includes attacker and attackOwner in the event message.
comment:17 by , 2 years ago
Milestone: | Backlog → Alpha 27 |
---|---|
Owner: | set to |
Patch: | → Phab:D4643 |
comment:18 by , 17 months ago
From #6675
When a ship is destroyed that had units garrisoned inside, these units disappear form the game (they've presumably died in the shipwreck) but they are not included in the lost unit counter in the stats. Is this intentional?
Screenshot from the stats of my playing Corsica and Sardinia for the first time in a while. You can see that the total of units lost from red and yellow are lower than the number of trained units.
Disclaimer: new to the forum (not new to the game, started playing in 2018). If this is not the right place for this kind of post, lmk. I sifted through the FAQ as well as this thread, and did a quick reddit search on r/0ad and did not see anything recent about this topic, but I'd be surprised if I was the first to bring it up. Game version: Alpha26
Summary din't contain units lost in building too (when no free space to place units before destruction), so patch fixes this too.
But there is one moment: if delete a unit/builduing manually, then it won't be in the summary, or it shouldn't be?