add item bulk forbid/dump/melt tool
#896
Conversation
chdoc
force-pushed
the
item
branch
2 times, most recently
from
c55fd23 to
9ec1c95
Compare
|
Obviously, this is still in a very rough shape. General feedback is welcome; let's not get lost in the nitty-gritty. |
There was a problem hiding this comment.
Overall, looks great!
There is overlap with buildingplan for the filtering by material/quality, but I don't know how much benefit there is to sharing that logic. Maybe if there are performance issues with filtering large amounts of items. Or maybe if you also want to offer category filters like "stone".
I also noted overlap with logistics that I feel more strongly should be shared
docs/item.rst
Outdated
| Usage | ||
| ----- | ||
|
|
||
| ``item [ help | count | [un]forbid | [un]dump | [un]melt ] <filter option>`` |
There was a problem hiding this comment.
"[un]trade" is notably missing, though that might be a v2 kind of thing. The code for dealing with trade items is all written, of course, but refactoring it so it's useful for this script will be a bit of work.
There was a problem hiding this comment.
Yes, this is basically why I stayed away from this for the moment. Really want this though.
There was a problem hiding this comment.
With most of the other action items taken care of, maybe this is the moment to take another look at trading. The main difference to the other actions is that trading is not local to the item, it also requires a trade depot. In my own forts, I never have more than one trade depot, so df.global.world.buildings.other.TRADE_DEPOT[0] would do (if completed). However, if there are multiple trade depots, one has to choose the right depot, no?
The logic in logistics.cpp looks dubious to me. It seems to check that there is a caravan approaching, and then return the first trade depot that is completely built and not being deconstructed. I don't see any connection between the selected depot and the approaching caravan.
|
more ideas for useful filters here: http://www.bay12forums.com/smf/index.php?topic=164123.msg8521571#msg8521571 |
chdoc
force-pushed
the
item
branch
2 times, most recently
from
c859649 to
9e6383f
Compare
|
I don't know why, but your comment about the API still needing to be documented does not offer me the option to reply/resolve. I have taken note of this, and I will document the API once we are in agreement what it should look like. |
I was actually hoping the documentation would facilitate the conversation. It's hard for me to understand your intent without seeing the docs about how you see the API being used. Edit: I didn't see your comment above discussing the API design -- I'll reply there |
chdoc
force-pushed
the
item
branch
3 times, most recently
from
0c1a4c1 to
a08bf68
Compare
docs/item.rst
Outdated
| ``reqscript('item')``: | ||
|
|
||
| * ``item.execute(action, conditions, options)`` | ||
| * ``item.executeWithPrinting(action, conditions, options)`` |
There was a problem hiding this comment.
| ``reqscript('item')``: | |
| * ``item.execute(action, conditions, options)`` | |
| * ``item.executeWithPrinting(action, conditions, options)`` | |
| ``reqscript('item')``:: | |
| local itemtools = reqscript('item') | |
| itemtools.execute(action, conditions, options) | |
| itemtools.executeWithPrinting(action, conditions, options) |
There was a problem hiding this comment.
I'm reluctant to call the variable that holds the reqscript return value "item" in the example since that is used in so many scripts as a local reference to an actual item.
docs/item.rst
Outdated
| * ``item.condition_forbidden(tab)`` | ||
| Checks for ``item.flags.forbid`` | ||
|
|
||
| * ``item.condition_melt(tab)`` | ||
| Checks for ``item.flags.melt`` | ||
|
|
||
| * ``item.condition_dump(tab)`` | ||
| Checks for ``item.flags.dump`` |
There was a problem hiding this comment.
shouldn't these take a boolean so the inverse can be matched?
There was a problem hiding this comment.
also, there seem to be a mix of condition.foo() + condition.notfoo() and condition.foo(bool). I'd prefer to standardize on the latter, taking a boolean to indicate whether the relevant property should be true or false. that is foo(true) should match foo being true. I'd prefer to avoid invert=true semantics, like what is currently on the stockpiled condition.
There was a problem hiding this comment.
I agree, the API for condition creation still looks somewhat haphazard and that condition.foo(...,bool) is the way to go. However, I would still advocate for negate=true semantics. This way, the argument can be naturally elided when the condition is to be matched positively, which is probably the majority of cases. Otherwise, this would require true and nil to behave the same and false to stand for negation.
item.lua
Outdated
| end | ||
| table.sort(sorted, function(a, b) return a.count > b.count end) | ||
| print(("\n%-14s %5s\n"):format("TYPE", "COUNT")) | ||
| for _, t in pairs(sorted) do |
There was a problem hiding this comment.
| for _, t in pairs(sorted) do | |
| for _, t in ipairs(sorted) do |
Co-authored-by: Myk <[email protected]>
Co-authored-by: Myk <[email protected]>
This adds a generic item manipulation tool, allowing to bulk forbid/dump/melt items based on various criteria or remove the aforementioned designations. Also provides an API to serve as backend for other scripts.