Ticket #44273

Lua: player losing method

Open Date: 2022-04-03 05:14 Last Update: 2022-04-26 15:45

Reporter:
Owner:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
2

Details

Yet another split of #44216. A method to make a player immediately losing a game, as like by losing a king. Since existing callbacks already can kill a king, this one won't create more potentially unstable code than it already exists.

Syntax: (Player):lose(Player looter?, bool loot?) (if looter is specified and loot is true or is not specified and gameloss style includes "Loot", looter gets some part of cities, map and gold of the player).

This method was intended to set PSTATUS_DYING to the player while further killing is done by kill_dying_players(), but actually I am not sure we call that function in a right time now (it goes when AIs start a phase, at phase end, and after handling any incoming game packet), so it should be thought about better.

Ticket History (3/16 Histories)

2022-04-03 05:14 Updated by: ihnatus
  • New Ticket "Lua: player losing method" created
2022-04-03 05:19 Updated by: cazfi
Comment

Reply To ihnatus

Syntax: (Player):lose(Player looter?, bool loot?) (if looter is specified and loot is true or is not specified and gameloss style includes "Loot", looter gets some part of cities, map and gold of the player).

For a parameter named 'looter', one would assume that nil would indicate that there's no looting, without extra bool parameter. What is the reason for having the boolean?

2022-04-03 07:00 Updated by: None
Comment

Reply To cazfi

Reply To ihnatus

For a parameter named 'looter', one would assume that nil would indicate that there's no looting, without extra bool parameter. What is the reason for having the boolean?

Well, probably it's not necessary if we don't want to implement multi-ruleset scripts (applying sometimes ruleset default behaviour and sometimes forcing looting). Finally, most of the looting (for now, except city transfer) can be done by other methods.

2022-04-06 05:55 Updated by: ihnatus
Comment

Wrote a patch but when I apply it to a connected player I got a warning on the server: Got a packet of type PACKET_PLAYER_PHASE_DONE(52) from a dead player

2022-04-07 05:07 Updated by: ihnatus
Comment

Actually, it's not this feature problem, killing a Leader by script also makes your client not aware that you are already dead. Likely, I'll improve this patch in other aspects and open a separate ticket.

2022-04-08 01:46 Updated by: cazfi
Comment

Reply To ihnatus

Likely, I'll improve this patch in other aspects

Just to confirm: so you don't consider this commit-candidate yet?

2022-04-08 02:03 Updated by: None
Comment

Reply To cazfi

Reply To ihnatus

Likely, I'll improve this patch in other aspects

Just to confirm: so you don't consider this commit-candidate yet?

The latter one is.

2022-04-08 02:05 Updated by: ihnatus
Comment

I just have not tested the patch on master, most probably it needs a bit of reworking.

2022-04-08 02:27 Updated by: cazfi
Comment

The patch looks good, compiles fine.

I just have not tested the patch on master, most probably it needs a bit of reworking.

At least it applies cleanly, in case you were referring to the possibility that the looting code you are moving around has changed (as it's removal from original location applies cleanly -> it has not changed)

Reply To ihnatus

Actually, it's not this feature problem, killing a Leader by script also makes your client not aware that you are already dead.

This makes me a bit uneasy. While the patch is not introducing the problem, it's clearly making it worse, and introducing more explicit way to end to the error situation. I would be much happier if the issue is resolved first, and this one would go in only after that.

I think I'll take this patch to my local queue already, but won't push it in to github just yet.

2022-04-08 05:15 Updated by: ihnatus
Comment

Reply To ihnatus

Likely, I'll improve this patch in other aspects and open a separate ticket.

--> #44284.

2022-04-15 19:06 Updated by: cazfi
Comment

I've looked around the related code a bit, and it seems to me that there's not good guarantees that we can get everything stable quickly.

Also: Requirements for including new features should should go only stricter as freeze approaches, and this is what I wrote last September: https://www.freelists.org/post/freeciv-dev/Towards-31

I hate to postpone features that people want, but if we never stop them flowing in to the "very next version" that very next version gets never released. 3.1 has been in development for far too long already (since 01-Jan-2017!). I really don't want to take any unnecessary risks of further delays with its schedule, i.e., potentially introducing regressions that block milestones going forward. We have a ton of work to do to get 3.1 released with its current feature set.

At this point my opinion is that this cannot be included in 3.1. I hope you understand.

2022-04-22 20:00 Updated by: cazfi
Comment

Reply To cazfi

Reply To ihnatus

Actually, it's not this feature problem, killing a Leader by script also makes your client not aware that you are already dead.

This makes me a bit uneasy. While the patch is not introducing the problem, it's clearly making it worse, and introducing more explicit way to end to the error situation. I would be much happier if the issue is resolved first, and this one would go in only after that.

With 3.1 target dropped, could we now take this in (to development master only)? For the above reason of knowingly making a problem worse it would be a bit against our usual policies.

I'm leaning just a bit towards taking this in, and making #44284 a S3_2-d3f blocker solvable by reverting this patch if everything else fails.

2022-04-24 14:50 Updated by: cazfi
  • Owner Update from (None) to cazfi
  • Resolution Update from None to Accepted
Comment

Reply To cazfi

I'm leaning just a bit towards taking this in, and making #44284 a S3_2-d3f blocker solvable by reverting this patch if everything else fails.

Thus setting this to review for inclusion to master.

2022-04-26 15:45 Updated by: cazfi
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Attachment File List

Edit

Please login to add comment to this ticket » Login