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

Drop players armor and weapons #1430

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

RedRafe
Copy link
Contributor

@RedRafe RedRafe commented Aug 21, 2024

Changes

  • fixed typo in configs offline_timout_mins --> offline_timeout_mins
  • added config to drop player's armor and weapons alongside player's inventory when logging off

This issue was raised on Admins chat to retrieve player's armor/weapons when players are banned, and also when players disconnect hoarding a lot of stuff. So the new config has a field startup_gear_drop_hours = 24 which means:

  • ANY player logging off in the first 24h will drop their armor alongside inventory/weapons
  • Rank::Regular and above will NOT drop their armor after first 24h of game (but they will still drop inventory (+weapons) as usual.
if not banned and player.connected then return end

I removed this check because it was not clear to me what edge case it was supposed to gate. The new conditions appeared to be always true so it seemed redundant, but happy to revert it.
Also I added a debug option to drop player's stuff in debug mode, but could be too much overhead so it can be removed if we prefer... I've tested it in all the cases (except ban) and it's unlikely I'll need to retest this often

screenshot of the changes
Screenshot from 2024-08-21 11-14-33

Copy link
Collaborator

@grilledham grilledham left a comment

Choose a reason for hiding this comment

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

I think this is mostly fine and a welcome improvement. As this is functionally used by multiple maps I want to play test it before I approve. And I want to wait for your thoughts on my comments before I play test.

@@ -22,59 +28,54 @@ local function spawn_player_corpse(player, banned, timeout_minutes)
local player_index = player.index
offline_player_queue[player_index] = nil

if not banned and player.connected then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this check was to cover the case were the player re-connects after the timer has been started. Which we don't care about if they were banned as we don't go through the timer in that case.

If you agree this is needed, I think we should add this check back.

Copy link
Contributor Author

@RedRafe RedRafe Aug 28, 2024

Choose a reason for hiding this comment

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

cover the case were the player re-connects after the timer has been started

Ah that might be it, thanks! Reverting

@@ -144,7 +145,12 @@ Event.add(defines.events.on_pre_player_left_game, function(event)
return
end

start_timer(event, config.offline_timout_mins)
if _DEBUG then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the debug code would make more sense in start_timer. I assume it's really the set_timeout_in_ticks you are trying to avoid for testing purposes. And if it's for testing it makes sense to run as much of the real code as possible.

Maybe something like this in start_timer would be better:

local data = {player = player, tick = tick, timeout_minutes = timeout_minutes}
if _DEBUG then
    local spawn_player_corpse_func = Token.get(spawn_player_corpse_token)
    spawn_player_corpse_func(data)
else
   set_timeout_in_ticks(timeout, spawn_player_corpse_token, data)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OKay I've decided I dont care about debug mode, and If we'll have to debug, just reduce the waiting time in configs or wait/speed up game if needed. So removed all the debug junk

features/dump_offline_inventories.lua Outdated Show resolved Hide resolved
@RedRafe
Copy link
Contributor Author

RedRafe commented Aug 21, 2024

So the biggest issue for me was testing this feature, because I had to start a SP game, save it, load it to our server, startup it and then join/leave. And it's tricky because in SP when you exit the game it gets paused, while in MP it register your leave. So my SP leave would actually trigger the 1st boot of the game on server side, and then trigger regularly when joining/leaving server as usual. The issue is that when the server boots up and the leave triggers, player.connect would be false, while when joining/leaving a running server, since it runs on PRE-player leave, that check sees player.connect = true.

This is just a big mess to correctly debug, and that's why I left the debug code in it (which I usually dont), BUT I dont like to leave it on, because it makes the actual working logic so much more complex to maintain/read IMO. Maybe I'd prefer to leave it in code as commented (so could be temporarily de-commented if we need to refactor/debug in the future cuz it would be helpful to have it there and not figuring out this big loophole again), or maybe figure a better workflow that would work all round. I also dont want it to run on _DEBUG = true cuz we unlikely will want to drop our invs when testing other stuff around the maps.

Maybe was just easier to leave the timeout thing as is and just speed up to 100x from console after leaving to simulate it (or reduce time to <1m), but yeah... didnt think of it before

@RedRafe
Copy link
Contributor Author

RedRafe commented Aug 21, 2024

Yes I forgot to mention in the refactor I wanted scenarios to be able to set timeout = 0 to let invs drop instantly when leaving and prev version would not allow it (I guess a workaround would be to set it to 1/60 or whatever <1 min so it just happens next tick or very shortly still which is good enough)

@RedRafe
Copy link
Contributor Author

RedRafe commented Aug 28, 2024

@grilledham reverted the check for online players; and removed the debug stuff: I dont think that is necessary to keep it and would just make debugging other scenarios tedious.

Copy link
Collaborator

@grilledham grilledham left a comment

Choose a reason for hiding this comment

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

Made some changes to set a default values just in case. I've tested and looks good. Thanks for doing this.

@grilledham grilledham merged commit d109a66 into Refactorio:develop Aug 28, 2024
1 check passed
@RedRafe RedRafe deleted the dump-player-armor branch September 2, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants