Opened 3 years ago

Last modified 3 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:
Priority: Must Have Milestone: Backlog
Component: Simulation Keywords: patch simple
Cc: Imarok Patch:

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)

garrison_depth.js.patch (796 bytes) - added by Vladislav Belov 3 years ago.

Download all attachments as: .zip

Change History (14)

Changed 3 years ago by Vladislav Belov

Attachment: garrison_depth.js.patch added

comment:1 Changed 3 years ago by Vladislav Belov

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?

Last edited 3 years ago by Vladislav Belov (previous) (diff)

comment:2 Changed 3 years ago by Vladislav Belov

Keywords: patch review added
Milestone: BacklogAlpha 21
Owner: set to Vladislav Belov
Status: newassigned
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

comment:3 Changed 3 years ago by elexis

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 in reply to:  3 Changed 3 years ago by Vladislav Belov

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.

comment:5 Changed 3 years ago by 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?

comment:6 in reply to:  5 Changed 3 years ago by Vladislav Belov

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 Changed 3 years ago by elexis

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 from Damage.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).
Last edited 3 years ago by elexis (previous) (diff)

comment:8 Changed 3 years ago by elexis

Keywords: simple added
Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:9 Changed 2 years ago by Imarok

Cc: Imarok added

comment:10 Changed 23 months ago by elexis

Owner: Vladislav Belov deleted
Priority: Should HaveMust Have
Status: assignednew

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 Changed 13 months ago by Mohamed Moanis

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 Changed 13 months ago by elexis

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 Changed 3 months ago by Imarok

Component: UI & SimulationSimulation

Move tickets to Simulation as UI & Simulation got some sub components.

Note: See TracTickets for help on using tickets.