Multi-filter for recipes in crafting menu #27500

OrenAudeles · 2019-01-08T16:48:13Z

Summary

SUMMARY: Interface "Allow multiple filters for crafting recipes in crafting menu"

Purpose of change

Current implementation disallows filtering on more than one criteria at once. It is therefore impossible to filter for all recipes that use leather that also cover the torso, or filter for all sock recipes that use yarn. Multiple filters gives more flexibility to how the crafting recipe set can be queried.

Describe the solution

Iterate over query string finding a chosen delimiter (comma) to generate query substrings to be processed. The first substring is processed using the current implementation to get an initial filtered recipe set. Remaining substrings are processed to reduce the current filtered recipe set towards a final filtered recipe set.

Additional context

Example usage of multiple filters to select only yarn-based socks:

jbytheway

A good feature that I would certainly like to exist. Some review comments about specific parts of the code below.

Also, the help message will need to be updated to explain how this new aspect works.

jbytheway · 2019-01-08T17:17:12Z

src/recipe_dictionary.cpp

+    }
+
+    return res;
+}


This new recipe_subset::search has a lot of code duplication with the existing one. It would be preferable to not duplicate this logic.

I think the simplest way to achieve that is to have the old function call the new one (rather than the other way around, as you currently have). Alternatively, both of them could call some third (new) function.

jbytheway · 2019-01-08T17:20:22Z

src/crafting_gui.cpp

+
+                                    // Populate if the first query filter, reduce otherwise
+                                    if( qry_begin == 0 ) {
+                                        picking.swap( available );


Using swap to assign a vector is entirely correct, but no longer necessary in a C++11 world. It would probably be less confusing to move-assign.

(Similarly for col_result below)

jbytheway · 2019-01-08T17:21:35Z

src/crafting_gui.cpp

+                                    if( qry_begin == 0 ) {
+                                        picking.swap( available );
+                                    } else {
+                                        std::set_intersection( available.begin(), available.end(), picking.begin(), picking.end(),


col_result can be declared inside this else block.

jbytheway · 2019-01-08T17:30:17Z

src/recipe_dictionary.h

@@ -133,6 +133,10 @@ class recipe_subset
        /** Find recipes matching query (left anchored partial matches are supported) */
        std::vector<const recipe *> search( const std::string &txt,
                                            const search_type key = search_type::name ) const;
+        /** Find recipes matching query existing in subset of recipes (left anchored partial matches are supported) */
+        std::vector<const recipe *> search( const std::vector<const recipe *> subset,


It's a bit strange to be asking a recipe_subset object to filter a different subset of recipes. The name suggests that the recipe_subset already represents a subset of recipes. So this interface is confusing (though I don't blame you for taking this approach; it's straightforward).

I feel like a more logical solution (which requires more changes) is that recipe_subset::search should not return a vector<recipe*>, but instead return a new recipe_subset object representing the new, smaller subset. Then you would not need to add this new function; it would simply be a matter of repeated search calls. That would naturally fix some of my objections above.

This is just an idea; I haven't thought it through in detail; it may not work out in practice.

Works surprisingly well actually! Gets rid of that second search function, made a new function reduce that calls into search before returning a recipe_subset reducible by future reduce calls :D

jbytheway · 2019-01-08T17:33:46Z

src/recipe_dictionary.cpp

+            }
+        } );
+    } else { // Subset is empty, populate from recipe_subset data
+        res = search( txt, key );


If the subset is empty you are resetting the search to search from all known recipes once more? Perhaps I have misunderstood, but that seems like it could lead to oddities. If the first few search terms filter down to zero results, then suddenly it will expand again to all recipes matching the next filter, effectively ignoring those first few search terms.

Edit: I see you abort the loop once the subset is empty, so this shouldn't happen. It's still confusing code, though. Is this alternative branch really needed?

…new recipe_subset

jbytheway

Looking good.

jbytheway · 2019-01-08T20:15:33Z

src/recipe_dictionary.cpp

@@ -222,6 +168,37 @@ std::vector<const recipe *> recipe_subset::search( const std::string &txt,

    return res;
 }
+static void insert_vector( const recipe_subset &src, recipe_subset &dst, const std::vector<const recipe *> &recipes ) {


This could be a recipe_subset constructor, rather than a static member function (Assuming I'm correct in thinking that you only ever use it to insert into an empty recipe_subset).

Your assumption is correct, only ever adding to an empty recipe_subset. A constructor would take the const recipe_subset &src and const std::vector<const recipe *> &recipes arguments and transplant the functionality I guess.
recipe_subset doesn't currently have a constructor so I would need to define additional constructors (empty/copy at minimum). This is the only situation where we have a possible want for a new constructor, and at the moment would only be useful in the reduce/intersection/difference functions.
Unless there is a broader requirement for new constructors I am reluctant to add one.

recipe_subset doesn't currently have a constructor so I would need to define additional constructors (empty/copy at minimum). This is the only situation where we have a possible want for a new constructor, and at the moment would only be useful in the reduce/intersection/difference functions.

You don't need to add a copy constructor; the default continues to exist even if you define others. And the default constructor is just a one line recipe_subset() = default;.

Not vital; I think it would be neater, but that's subjective.

ah okay, then my argument against is mostly moot. I'll try out the addition of recipe_subset() = default; and recipe_subset( const recipe_subset &src, const std::vector<const recipe *> ); constructors. Probably is neater, at the very least it should clean up the reduce/intersection/difference functions a little.

OrenAudeles added 6 commits January 8, 2019 09:07

allow multiple recipe filters in crafting menu

b1572c2

astyle

a2d66b6

Remove unnecessary variable controlling filter loop

7b039c7

Reduce code duplication in filter loop

db8ddc1

Fix initial population if 'm' or 'h' are first filters

8384a14

End filter loop if zero results found at any point

c10c6b9

jbytheway reviewed Jan 8, 2019

View reviewed changes

OrenAudeles added 3 commits January 8, 2019 11:47

Update filter description to include multi-filter delimiter

61d1bc2

Filter loop now uses reduce/intersection/difference functions to get …

46493a2

…new recipe_subset

astyle

74604c1

jbytheway reviewed Jan 8, 2019

View reviewed changes

Add new recipe_subset ctor, use it to clean up filter functions

05afce5

OrenAudeles force-pushed the crafting_multi_filter branch from a9cea01 to 05afce5 Compare January 9, 2019 23:32

OrenAudeles changed the title ~~[WIP] Multi-filter for recipes in crafting menu~~ Multi-filter for recipes in crafting menu Jan 10, 2019

ZhilkinSerg added <Enhancement / Feature> New features, or enhancements on existing Info / User Interface Game - player communication, menus, etc. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [C++] Changes (can be) made in C++. Previously named `Code` labels Jan 10, 2019

kevingranade merged commit 298be7b into CleverRaven:master Jan 12, 2019

OrenAudeles deleted the crafting_multi_filter branch January 12, 2019 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-filter for recipes in crafting menu #27500

Multi-filter for recipes in crafting menu #27500

OrenAudeles commented Jan 8, 2019

jbytheway left a comment

jbytheway Jan 8, 2019

jbytheway Jan 8, 2019

jbytheway Jan 8, 2019

jbytheway Jan 8, 2019

OrenAudeles Jan 8, 2019

jbytheway Jan 8, 2019

jbytheway left a comment •

edited

jbytheway Jan 8, 2019

OrenAudeles Jan 9, 2019

jbytheway Jan 9, 2019 •

edited

OrenAudeles Jan 9, 2019

Multi-filter for recipes in crafting menu #27500

Multi-filter for recipes in crafting menu #27500

Conversation

OrenAudeles commented Jan 8, 2019

Summary

Purpose of change

Describe the solution

Additional context

jbytheway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway left a comment •

edited

jbytheway Jan 9, 2019 •

edited