Skip to content

Fix state inconsistency in uniform-unstick #1490

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 18, 2025
Merged

Fix state inconsistency in uniform-unstick #1490

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdf3ebb
Fix uniform assignment state inconsistency caused by uniform-unstick
Aug 13, 2025
fb3f2d1
Refactor item description logging in uniform-unstick
Aug 13, 2025
0831e6e
Clean up/improve code readability in uniform-unstick
Aug 17, 2025
5b4072e
Merge branch 'master' into uniform-unstick-fix
ab9rf Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions uniform-unstick.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ local validArgs = utils.invert({
-- Functions

local function item_description(item)
return dfhack.df2console(dfhack.items.getDescription(item, 0, true))
return "item #" .. item.id .. " '" .. dfhack.df2console(dfhack.items.getDescription(item, 0, true)) .. "'"
end
Comment on lines 20 to 22
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extending item_description() to gracefully handle the case of a nonexistent item.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear on what you mean here by the "case of a nonexistent item"

Copy link
Contributor

Choose a reason for hiding this comment

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

The case handled on line 208. Literally df.item.find() returned nil.

Copy link
Member

Choose a reason for hiding this comment

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

there is no "valid" response to being asked to describe a null item; the correct response is for the function to throw, which is what it currently does

callers are expected to check for nil or be prepared to accept a throw if they do not

Copy link
Member

Choose a reason for hiding this comment

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

I did a cursory scan through the code and I think item will never be null on calls to this function, but I am not at all sure of that. If you're confident that this code will never be called with item as null, it's enough to document that item must never be null. Otherwise, add a test for item being null to this code and provide an appropriate alternative. Otherwise, the script will abort with an exception, which will generally be confusing to users (as most people never look at the console and thus won't see the exception).


local function get_item_pos(item)
Expand Down Expand Up @@ -166,11 +166,10 @@ local function process(unit, args, need_newline)
for u_id, item in pairs(assigned_items) do
if not worn_items[u_id] then
if not silent then
need_newline = print_line(unit_name .. " is missing an assigned item, object #" .. u_id .. " '" ..
item_description(item) .. "'", need_newline)
need_newline = print_line(unit_name .. " is missing an assigned item, " .. item_description(item), need_newline)
end
if dfhack.items.getGeneralRef(item, df.general_ref_type.UNIT_HOLDER) then
need_newline = print_line(unit_name .. " cannot equip item: another unit has a claim on object #" .. u_id .. " '" .. item_description(item) .. "'", need_newline)
need_newline = print_line(unit_name .. " cannot equip item: another unit has a claim on " .. item_description(item), need_newline)
if args.free then
print(" Removing from uniform")
assigned_items[u_id] = nil
Expand Down Expand Up @@ -204,9 +203,9 @@ local function process(unit, args, need_newline)
if assigned_items[u_id] == nil and u_id ~= squad_position.equipment.quiver and u_id ~= squad_position.equipment.backpack and u_id ~= squad_position.equipment.flask then
local item = df.item.find(u_id)
if item ~= nil then
need_newline = print_line(unit_name .. " has an improperly assigned item, item # " .. u_id .. " '" .. item_description(item) .. "'; removing it")
need_newline = print_line(unit_name .. " has an improperly assigned item, " .. item_description(item) .. '; removing it')
else
need_newline = print_line(unit_name .. " has a nonexistent item assigned, item # " .. u_id .. "; removing it")
need_newline = print_line(unit_name .. " has a nonexistent item assigned, item # " .. u_id .. '; removing it')
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not passing in need_newline to print_line(). That's harmless; it tests nil instead of false, but it's inconsistent with the API. I'm not sure it's worth fixing.

You changed from double to single quotes; I'm not sure why. Everywhere else uses double quotes. Harmless.

end
squad_position.equipment.assigned_items:erase(i)
end
Expand Down Expand Up @@ -240,9 +239,7 @@ local function process(unit, args, need_newline)
for w_id, item in pairs(worn_items) do
if assigned_items[w_id] == nil then -- don't drop uniform pieces (including shields, weapons for hands)
if uncovered[worn_parts[w_id]] then
need_newline = print_line(unit_name ..
" potentially has object #" ..
w_id .. " '" .. item_description(item) .. "' blocking a missing uniform item.", need_newline)
need_newline = print_line(unit_name .. " potentially has " .. item_description(item) .. " blocking a missing uniform item.", need_newline)
if args.drop then
to_drop[w_id] = item
end
Expand All @@ -261,12 +258,12 @@ local function do_drop(item_list)
for id, item in pairs(item_list) do
local pos = get_item_pos(item)
if not pos then
dfhack.printerr("Could not find drop location for item #" .. id .. " " .. item_description(item))
dfhack.printerr("Could not find drop location for " .. item_description(item))
else
if dfhack.items.moveToGround(item, pos) then
print("Dropped item #" .. id .. " '" .. item_description(item) .. "'")
print("Dropped " .. item_description(item))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the need_newline = print_line() abomination, should it not?

EDIT: no, it makes no difference here. That's gonna be a rant by itself.

else
dfhack.printerr("Could not drop object #" .. id .. " " .. item_description(item))
dfhack.printerr("Could not drop " .. item_description(item))
end
end
end
Expand Down
Loading