Fix state inconsistency in uniform-unstick #1490
Conversation
|
May need additional handling for items that have been offloaded to |
There was a problem hiding this comment.
This fixes the bug you're targeting; it's fine.
I still think we should totally construct a copy of the data from scratch, and compare with the existing vector, but I know that's extra work for (probably) no gain.
I just see three possibilities for corruption:
- values that shouldn't be in this vector. You're fixing that.
- values that should be in the vector but aren't. I haven't probed for this, but it's probably not happening.
- the vector itself not being sorted. I did check two forts with extensive military (in 0.47.05 and 0.51.*), and didn't find any cases of this.
I suppose this is just nitpicking. So is my line-by-line; there's nothing that needs changed.
uniform-unstick.lua
Outdated
| @@ -196,6 +196,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] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| else | ||
| if dfhack.items.moveToGround(item, pos) then | ||
| print("Dropped item #" .. id .. " '" .. item_description(item) .. "'") | ||
| print("Dropped " .. item_description(item)) |
There was a problem hiding this comment.
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.
uniform-unstick.lua
Outdated
| 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') |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Consider extending item_description() to gracefully handle the case of a nonexistent item.
There was a problem hiding this comment.
I'm unclear on what you mean here by the "case of a nonexistent item"
There was a problem hiding this comment.
The case handled on line 208. Literally df.item.find() returned nil.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
|
Did a quick readability pass on the script as a whole. Ultimately with how many different structures this script is pulling from and comparing I'm not sure it's ever going to reach fantastic readability, but hopefully this is at least a decent step in the right direction. |
fixes DFHack/dfhack#5460