-
Notifications
You must be signed in to change notification settings - Fork 492
orders: add search overlay for manager orders #5677
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.
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
base: develop
Are you sure you want to change the base?
Conversation
Adds search overlay to find and navigate manager orders with arrow indicators showing current search result. Search uses Alt+S to focus, Alt+P/N for prev/next navigation. Overlays are disabled by default.
ChrisJohnsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice workaround for the inability to filter the view like in the other info panel search widgets.
I like the labeled magic numbers in OrderHighlightOverlay. DF's layout behavior is not fun thing to have to replicate, but I think this would be fairly clear to someone that hasn't looked at this stuff as much (at least as long as they have DF at hand to try out different window sizes).
Re: not being able to detect exiting the window: yeah, I have also wanted something
like a callback for "this overlay is no longer active (not going to be rendered)".
- Would utils.search_text be useful here? The main SortOverlay uses it for searching.
- Could the first match automatically be highlighted on Enter?
I can see how you might not want to highlight (thus likely move the scroll position) for each change in search input, but Enter seems like a nice place to jump to the first match. - The highlight should probably be cleared when the search text changes (especially when the highlighted order does not match the new search input).
- It might just be my color vision being flaky, but I completely did not notice the highlight arrow at first. Now that I know what to look for, it isn't hard to spot. But my first cycling through the matches was confusing because it wasn't obvious which order was being indicated.
Now there is 16 visible chars in search
I switched to using utils.search_text instead of the custom search logic
Added Enter/Shift+Enter to cycle through matches using the default submit/submit2 methods.
Fixed, now the highlight gets cleared whenever the search text changes.
I reshaped the arrow, moved it to the right side of icon and used more contrasting colors (black on white) to make it more visible. @ChrisJohnsen Thanks for all the detailed feedback! I made each change in a separate commit to make the review easier. Let me know if there's anything else that needs adjusting. |
ChrisJohnsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spotting of the need to hide when job_details.open. The other functionality changes all seem good (with a new note about order deletion handling).
I've put some more thoughts on the specifics of the code in this review.
|
So, I hadn't looked into the details of But those The other caller is |
… results after sorting/clearing/importing orders
…r missing entries
…le-locals and inline arrow colors
…Overlay render. Return when disable cursor
I added df.delete(reaction.order) to free the C++ objects and set reactions = nil afterward. I'm not very familiar with memory leak issues, so please let me know if this fix looks correct or if there's anything else I should do. |
|
I need advice on where to put stockflow collect_reactions(): Option 1: Use
Option 2: Use
Option 3: Create a new
What approach would you recommend to get reactions? |
ChrisJohnsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found couple of functionality things that I didn't notice before:
- cache invalidation between world loads,
- searchability after setting material for an order.
While looking at the details of customized orders I wondered if the overall size of the cache can be reduced.
Some functions can be made local (they don't access self, or won't once the methods the call are themselves made local):
perform_searchgetListStartYgetViewportSizecalculateSelectedOrderY
When adding/removing an order there is still a small UI bug: the "Search: X of Y" title remains unchanged even if a matching order is added or removed. You could refresh the search when the number of orders changes, or just revert to the plain "Search" title so that the (possibly) stale count isn't displayed.
| encrust_str) | ||
| end | ||
|
|
||
| local reaction_map_cache = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried loading different worlds I found that the cache wasn't being flushed like I expected. I mistakenly thought that a local cache variable would suffice. (I blame working too much on just overlays in script modules where a rescan will reload the module — when the script has been modified.)
The reaction cache needs to be flushed when a (new) world is loaded so that the world- or civilization-specific (e.g., clothing_reactions) orders can be cached.
For example, I have two forts: "A" knows how to make dresses but not shirts, "B" knows how to make shirts but not dresses. If I load "A" first, unload it, then load "B", searching "shirt" will not find a "make yarn shirt" order (though it does find still find a "forge iron mail shirt" order). This is because the "shirt" armor subtype was not cached since it was not available in fort "A".
Additionally, the cache should probably be a global variable. Leaving it as a local would force a rebuild after a module reload (even if the world hasn't changed).
You can watch for world loads with dfhack.onStateChange. The typical usage works like this:
At the top (after the require imports):
local GLOBAL_KEY = 'orders'
The local can be replaced with a global initialization that copies any existing value instead of starting off as nil:
reaction_map_cache = reaction_map_cache or nil
At the end (between OVERLAY_WIDGETS and return _ENV):
dfhack.onStateChange[GLOBAL_KEY] = function(sc)
if sc ~= SC_WORLD_LOADED then return end
reaction_map_cache = nil
get_cached_reaction_map()
end
| return reaction_map_cache | ||
| end | ||
|
|
||
| local function get_order_search_key(order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I take a basic "make wooden earring" order and use the DF UI to specify a wood type, the order becomes unsearchable. (using the shorter key strings described in another section of this review)
- make wooden earring:
235:-1:-1:-1:-1::2: - make willow earring:
235:-1:-1:420:203::2:
Only the generic "wooden earring" order is generated by collect_reactions, so the key for the order with willow specified is not found in the cache.
Rock pots works similarly, but the generated order is only generic on the mat_index:
- make rock pot:
216:-1:12:0:-1::0: - make microcline pot:
216:-1:12:0:233::0:
I experimented (in get_order_search_key) with checking for "genericized" orders (added material value "override" arguments to make_order_key) when the direct order is not found (first only with mat_index == -1, then with also with mat_type == -1 if there was still no match). When such a genericized match was found I extended the "search_key" with a material description fetched with
dfhack.matinfo.decode(order.mat_type, order.mat_index):toString()
This made the order searchable by its generic wording and its material description. The "search_key" ends up being "make wooden earring willow" but searching for "willow ear" (or "ow ear" with full text searching enabled) works due to utils.search_text tokenizing the sought text.
I have never needed to directly use material values before, so I am not sure if this "genericizing" is correct for all materials/items/jobs. It seems okay from how collect_reactions works (mat_type and mat_index default to -1, and sometimes only mat_type is specified), but I don't know the extent of what DF allows. Hopefully someone with more experience there can weight in here.
| local search_last_scroll_position = -1 | ||
| local order_count_at_highlight = 0 | ||
|
|
||
| local function make_order_key(order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was diagnosing the inability to search for orders with specified materials I ended up looking at a bunch of these order keys strings.
The last two components of the key are basically just expanded bitfields that list the flag names (and values for the material categories). It seems like these could just be represented by their (masked) integer values instead of the verbose "flag name" version.
The (current) "flag name" version of "Make wooden earring" looks like this (182 bytes for the key):
235:-1:-1:-1:-1::bone=false;cloth=false;horn=false;leather=false;pearl=false;plant=false;shell=false;silk=false;soap=false;strand=false;tooth=false;wood=true;wood2=false;yarn=false;: = make wooden earring
Using the integers for the flags gives this (19 bytes for the key):
235:-1:-1:-1:-1::2: = make wooden earring
In one of my forts, the total length of the "flag name" keys for the 3533 generated orders was 653,367 bytes. With the mostly-integer keys it was only 66,632.
I extracted the integer values like this:
local mat_cat = 0
for flag_name, flag_value in pairs(order.material_category) do
if type(flag_name) == 'string' and flag_value == true then
mat_cat = mat_cat | 1 << df.job_material_category[flag_name]
end
end
local encrust_str = ''
if order.job_type == df.job_type.EncrustWithGems
or order.job_type == df.job_type.EncrustWithGlass
or order.job_type == df.job_type.EncrustWithStones
then
encrust_str = tostring(order.specflag.encrust_flags.whole
& (1 << df.stockpile_group_set.finished_goods
| 1 << df.stockpile_group_set.furniture
| 1 << df.stockpile_group_set.ammo))
end
Not only does this give shorter keys, but it is faster (250-400ms; "long keys" was 550-650ms).
It can be even faster by only computing the appropriate masks once (140-250ms).
There doesn't appear to be much support for "multi-field" manipulation of bitfields from the Lua side (a couple of functions in utils, but none that generates masks). It isn't too bad here because all the flags in question all single bits, but that isn't true for all bitfields.
The masking is required for the encrust flags (since we only want to pick a few of them), but it is probably a good idea for the material category flags (even though we want to know about all of them) to avoid picking up "stray bits" from the undefined portion of the bitfield (though this probably shouldn't happen...).
Whether or not the shorter keys are adopted, the falsity checks for material_category, specflag and specflag.encrust_flags are not needed since they are not behind pointers. If those fields are actually missing (renamed, moved, ...) even trying to check them for nil will raise an error.
Also, specflag is a union, so it isn't really appropriate to access specflags.encrust_flags without checking whether that member is the active variant. In the code above I checked the job_type, but I don't really know if that is the right "tag" to check before using the encrust_flags variant (it does seem to be the main thing collect_reactions sets when also setting encrust_flags).
I don't think there is a reason to try to use this version. Like you noted, the plugin is disabled. Its Lua code includes stuff that you don't need here.
I haven't really looked at scripts#1524 much yet. If it is causing problems for Quickfort, that will surely need to be addressed though. Quickfort is the only existing user of I don't know which exceptions you have seen from Quickfort, but it might be possible to make fewer changes to stockflow (keeping Quickfort working without changes) and make more adaptations in the order searching code (like extending "make earring" to "make earring willow" instead of making stockflow generate an additional "Make willow Earring" order).
Another copy of the code (e.g., with the scripts#1524 changes) probably isn't be best approach, if that's what you mean. It does seem the approach here of matching pre-described orders is kind of brittle. I suppose quickfort only needs to concern itself with the list of items/orders that it supports placing, so it doesn't need to support all possible user-configurable, mod-enabled, world-varying orders. If there was some other approach for recreating the UI text DF would display for any given order, that might work better than trying to match order attributes against a predefined, cached list. That would probably take some reverse engineering work to suss out what all DF considers when rendering the order names. |

Add search to manager orders
Adds search box to find orders by job name + arrow indicators to show which one you're looking at.
What it does
Implementation
OrdersSearchOverlayfor search UI,OrderHighlightOverlayfor arrowsimportexportoverlay position to make room for search (and make new version of config)What didn't work
Related issues
order-search.mp4