Repository navigation
Add "civ styles" GUI - #1603
Add "civ styles" GUI#1603Snowstar38 wants to merge 2 commits into
Conversation
|
First of, thank you for contributing, this is a significant piece of work. At first glance, this looks to be in reasonably good shape. But the devil is usually in the detail. I'm still expecting this to require multiple rounds of code review before it can get merged. Given our currently limited reviewing capacity, this may take weeks or even months. Please indicate the level of AI-support that was used in the development of this tool (if any) and your willingness to iterate on this for several weeks? I would like to avoid spending hours on an in-depth code review, just for another pull request to get abandoned. |
chdoc
left a comment
chdoc
left a comment
There was a problem hiding this comment.
Here are some general remarks for the first round. I will do a more in-depth review once these are taken care of.
| @@ -0,0 +1,30 @@ | |||
| gui/civ-styles | |||
There was a problem hiding this comment.
Why call is tool civ-styles? the concept is already known as recipes. I would think the tool would be better named gui/recipes.
| it did not, ``gen`` for procedurally generated items, and ``xN`` if the style is | ||
| listed more than once, which repeated `add-recipe` runs will do. | ||
| :kbd:`Ctrl`:kbd:`D` removes duplicates. |
There was a problem hiding this comment.
Instead of fixing this behavior after the fact, please file a bug report against add-recipe.
| local civ_id = df.global.plotinfo.civ_id | ||
| if not civ_id or civ_id < 0 then return nil end |
There was a problem hiding this comment.
civ_id is an integer that is always defined when plotinfo is defined (basically always). So it is fine to directly call find with it.
| local cat = self.by_key[cat_key] | ||
| local vec = cat and self:get_vec(cat) | ||
| if not vec then return end | ||
| if count_in(vec, subtype) == 0 then |
There was a problem hiding this comment.
If all you need is a membership test, please use utils.linear_index(vector,key[,field]) and remove count_in.
|
Thanks for looking at it - sorry, I've had a lot going on and missed the notification email for this. The tool was written with AI assistance. The design is mine, I wanted to see and remove civ style types, not just add them, since On iterating: yes, I'm definitely willing to. Will be getting time this week to be able to rename it and polish anything that needs it. |
Review feedback from chdoc on DFHack#1603: - Rename the tool to gui/recipes, since "recipes" is the established name for the concept (add-recipe). Renames the script, the docs file, the changelog entry, the classes and the focus path, and switches the user-facing strings from "style" to "recipe". - get_civ(): plotinfo.civ_id is always a defined integer, so call df.historical_entity.find with it directly. - Use utils.linear_index for the membership test in Recipes:add and remove count_in. Also narrows one docs claim: duplicate entries come from "add-recipe single", which inserts without a duplicate check; the "all"/"exotic" path already guards with checkKnown.
Review feedback from chdoc on DFHack#1603: - Rename the tool to gui/recipes, since "recipes" is the established name for the concept (add-recipe). Renames the script, the docs file, the changelog entry, the classes and the focus path, and switches the user-facing strings from "style" to "recipe". - get_civ(): plotinfo.civ_id is always a defined integer, so call df.historical_entity.find with it directly. - Use utils.linear_index for the membership test in Recipes:add and remove count_in. Also narrows one docs claim: duplicate entries come from "add-recipe single", which inserts without a duplicate check; the "all"/"exotic" path already guards with checkKnown.
da9e05e to
d533dcd
Compare
|
Thanks for the review. All four are done, but I did have a question about the last one.
On dropping the dedupe feature - I think it could make sense to leave it, though I'd like to hear your thoughts? After add-recipe is fixed, duplicates ideally won't happen anymore, but some people might end up with saves that do have them. If you think it's better off without I'll remove it tho I loaded the renamed version in game and checked it: the window opens, categories populate, toggling works, and Also rebased onto current master, so the branch isn't 126 commits behind anymore. Disclosure: I used AI assistance (Claude) for this round of changes and for the write-up in #6022. The testing above is mine. |
2 participants
Adds gui/civ-styles, letting people edit the item styles a civilization knows how to make.
"add-recipe" can only add; there's no way to see what your civ already knows or remove any styles. This lists every style per category, marks which ones came from the civ's raws [helpful for modded civs/races], and lets you take styles away as well as grant them.
It also flags duplicate styles and can clear them (in case add-recipe was accidentally called 2x, lets users fix duplicate entries in menus). There are toggles to show known, unknown, etc.
Covers: armor, helms, gloves, shoes, pants, shields, weapons, diggers, training weapons, ammo, siege ammo, trap components, tools, toys, and instruments.