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

Show file tree
Hide file tree
Changes from 2 commits
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
1 change: 1 addition & 0 deletions changelog.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Template for new versions:
## New Features

## Fixes
- `uniform-unstick`: no longer causes units to equip multiples of assigned items

## Misc Improvements

Expand Down
33 changes: 23 additions & 10 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 All @@ -196,6 +195,22 @@ local function process(unit, args, need_newline)
end
end

-- Make the equipment.assigned_items list consistent with what is present in equipment.uniform
for i=#(squad_position.equipment.assigned_items)-1,0,-1 do
local u_id = squad_position.equipment.assigned_items[i]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'll just say that I don't like the variable name u_id. I would probably use item_id or aitem_id instead.

u_id is worse than meaningless, it's misleading. It makes me think it's a unit.id instead of an item.id.

I know it's used above, but even considering that, you're using it for a slightly different meaning. Above it means that an item is in the assigned_items hash key/value map, and therefore is in the uniform nested structure; here it means each item in the assigned_items vector.

I mean, it works, it's just not as clear as it could be. And this script really needs some clarity.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that u_id is a poor name for this variable. Would suggest uniform_item_id or something along those lines. Terseness is not a virtue. Feel free to break the if statement on line 203 over multiple lines.

-- Quiver, backpack, and flask are assigned in their own locations rather than in equipment.uniform, and thus need their own checks
-- If more separately-assigned items are added in the future, this handling will need to be updated accordingly
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_description(item) .. '; removing it')
else
need_newline = print_line(unit_name .. " has a nonexistent item assigned, item # " .. u_id .. '; removing it')
end
squad_position.equipment.assigned_items:erase(i)
end
end

-- Figure out which worn items should be dropped

-- First, figure out which body parts are covered by the uniform pieces we have.
Expand Down Expand Up @@ -224,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 @@ -245,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