[buildingplan] step4: generalize buildingplan to handle all building types#1675
Conversation
but restrict to only the currently supported set so we can still assume only one filter is required for each building. changes: - update buildingplan plugin version to 2.0 - new serialization format for planned buildings - old persistent data is automatically migrated to new format on load - algorithm now respects job_item filters; items must match job_item filter and buildingplan ItemFilter - more invalid items are now filtered out, like items encased in ice. are there any others we should be checking (see BadFlags struct) - items are sorted before job is unsuspended so final item ordering is correct regardless of what order the items were matched and attached - item counts in filters are kept up to date so if buildingplan is disabled before all filters are matched and the building is completed by DF itself, the item counts will come out correct (though the item ordering and building "roughness" may be incorrect) - fixes two memory leaks in building finalization code - allows artifacts to be matched (ItemFilter defaults now top out at Masterful -- Artifact is selectable but must be manually specified) - add gui to switch between items for buildings that require multiple item types
| { | ||
| auto max_quality = &getDefaultItemFilterForType(type)->max_quality; | ||
| *max_quality = static_cast<df::item_quality>(*max_quality + amount); | ||
| if (job_item->item_type > -1 && job_item->item_type != item->getType()) |
There was a problem hiding this comment.
these checks feel like they should be part of isSuitableItem() or maybe ItemTypeInfo::matches(). Is there any reason they don't check basic attributes like item type? or do they require that the item vector be set correctly, and that all items in that vector will have the same type?
There was a problem hiding this comment.
Not sure I know enough to answer your question, sorry. It looks like
dfhack/library/modules/Items.cpp
Line 282 in 58917b0
dfhack/library/modules/Items.cpp
Line 261 in 58917b0
There was a problem hiding this comment.
When I traced through the code, it seemed to me what was happening was that if any item in the vector was the appropriate type, then matches() would assume that every item in the vector was an acceptable type. This is a problem for any filter that doesn't have a vector_id set in buildings.lua, like any of the tools. The matches() function was checking IN_PLAY and deciding every item in the game could be used (that happened to match all the flags). I was getting hives built directly out of bars of coal before I added these checks. At one point I was trying looking for all items in IN_PLAY, and I was getting all furniture built out of bars of coal (I had createitem'd a lot of coal in the fort)
Adding the checks in buildingplan solves the issue for my use case, of course, but I can't help but think the are other code paths that would benefit from proper item matching.
There was a problem hiding this comment.
I was going to say that I bet advfort has this bug, but I see that isSuitableItem is locally implemented for that script (though the library isSuitableItem() and advfort's isSuitableItem() check different flags). In fact, the library implementation looks like it should have a lot of what advfort's isSuitableItem() function does. For example, if the job_item.flags2.building_material is set, why isn't isSuitableItem() or ItemTypeInfo::matches() calling item->isBuildMat()? Am I just misunderstanding how the library isSuitableItem() is supposed to be used?
There was a problem hiding this comment.
I added a few more relevant checks here (including the missing call to isBuildMat())
There was a problem hiding this comment.
The main difference I see from a glance is that advfort's isSuitableItem takes the item, and Jobs::isSuitableItem just takes an item type+subtype, so advfort's has the ability to call some more item methods. I'm not sure if Jobs::isSuitableItem is intentionally limited or if it was just written under the assumption that the item type+subtype would be enough.
I'm not very familiar with some of the code in these modules, so I'll defer to you if you think improvements are in order, but I think the checks you added make sense where you added them (at least given the current API).
myk002
changed the title
| { | ||
| auto max_quality = &getDefaultItemFilterForType(type)->max_quality; | ||
| *max_quality = static_cast<df::item_quality>(*max_quality + amount); | ||
| if (job_item->item_type > -1 && job_item->item_type != item->getType()) |
There was a problem hiding this comment.
Not sure I know enough to answer your question, sorry. It looks like
dfhack/library/modules/Items.cpp
Line 282 in 58917b0
dfhack/library/modules/Items.cpp
Line 261 in 58917b0
No, this PR doesn't expose support for any new building types. All changes are "under the hood" |
stolen (with love) from advfort.lua
2 participants
#1640
but restrict to only the currently supported set so we can still assume only one filter is required for each building.
Code can be reviewed after #1674 is merged.
changes: