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

Rich item copy for inventory drops #1346

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Soggs
Copy link
Contributor

@Soggs Soggs commented Feb 10, 2023

Any item-with-data now keeps that when being moved to the corpse. This preserves equipment grids, spidertron remote settings, etc.

Adds an option to exclude items from dropping.

Selection tool items (blueprints, deconstruction planner, upgrade planner) and blue print books are always excluded from dropping.

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.

Thanks for the PR. A few minor issues that need fixing before we can merge.

local offline_player_queue = {}

Global.register({offline_player_queue = offline_player_queue}, function(tbl)
offline_player_queue = tbl.offline_player_queue
config = Config.dump_offline_inventories
end)

local function move_items(source, target)
if not source.is_empty() then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not source.is_empty() then
if source.valid and not source.is_empty() then

I think with the way you changed the checks it's technically possible to get here with only one of the inventories being valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we never let go of control between obtaining the reference and using it, it should never become invalid. However we do need a nil check at that point. Which gives me an idea to rework the whole control flow of how we handle things. So I will come back to it.

@@ -11,13 +11,30 @@ local Config = require 'config'
local set_timeout_in_ticks = Task.set_timeout_in_ticks
local config = Config.dump_offline_inventories

local ignored_items_set = {}
for _ , k in pairs(config.ignored_items) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for _ , k in pairs(config.ignored_items) do
for _ , k in pairs(config.ignored_items or {}) do

There are a lot maps that directory set dump_offline_inventories with a new table that are missing this new field, search for dump_offline_inventories =. Another option would be to go to all those sites and add an empty table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add that.

if not source.is_empty() then
for i = 1, #source do
local stack = source[i]
if stack.valid_for_read and not stack.is_selection_tool() and not stack.is_blueprint_book() and not ignored_items_set[stack.name] then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if stack.valid_for_read and not stack.is_selection_tool() and not stack.is_blueprint_book() and not ignored_items_set[stack.name] then
if stack.valid and stack.valid_for_read and not stack.is_selection_tool and not stack.is_blueprint_book and not ignored_items_set[stack.name] then

According to the API these are boolean values not functions.
https://lua-api.factorio.com/latest/LuaItemStack.html#LuaItemStack.is_selection_tool
https://lua-api.factorio.com/latest/LuaItemStack.html#LuaItemStack.is_blueprint_book

Also I think bizarrely you have to check valid as well as valid for read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got carried over by the is_empty()

As for stack being invalid. That would imply that the inventory we pull from contains invalid references which should never be an constitute a major bug in Factorio.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There might be an edge case if the stack gets cleared on the same tick by another event. The stack might still be there but not valid? I'm not sure, but the Factorio devs always recommend adding the valid check and in this case there is no performance concern and it doesn't make the code more complicated. So, I'd be happier with the check, same above for the inventory check.

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