Show the recipes you know how to craft using an item as part of the item description

[yobbobanana]

I also ended up yanking some of the crafting stuff out of game and putting it in player, where it made slightly more sense.

This is currently pretty slow… not sure how much can be done about that, but as it only does it when you look at the description for an item perhaps it doesn't matter too much?

It only displays for components, not tools. Recipes you can currently craft are highlighted.

I tried caching the recipe lookup but it turns out the slow part is checking recipe craftability.

[KA101]

Welcome back, yobbo.

[kevingranade]

Seconding the welcome, nice to see you back.
If it's slow, how about only doing it on demand? 'what can I craft button'

[yobbobanana]

Turns out i was doing it wrong!

It's faster now, but does still cause a noticable delay. Might get bad with a high level character with huge stash, but I don't have one to test at the moment.

I'm tempted to do some sort of "advanced examine item", the item descriptions have really gotten detailed, even without this addition. But that does look a little daunting.

[BevapDin]

It seems this is recipes_by_component. Otherwise: what itype is meant here? The tools, the components, the byproducts or the product?

[yobbobanana]

Yeah recipes_by_component is better, i'll change that.

[BevapDin]

Should definitely take a reference to const, the item is not supposed to be changed here.

[BevapDin]

Why this change? The new name is redundant, it repeats the name of the class. There is no other loading function that it needs to be distinguished from either. To me this looks like naming the length function std::string as string_length, it just adds redundancy. Of course requirements::load loads the requirements, what else should it load?

And the comment above that line is also redundant, it's word by word the same as the code: rec == recipe, load_requirements == load requirements, what's the point of it?

Why not make it even more explicit: load_requirements_from_json_given_as_parameter_into_this_object(...) or player::can_player_disassemble?

[yobbobanana]

That method isn't actually part of the recipe class, it's part of the requirements class, which is inherited by recipe. So when it's called just as "load" in recipe it's unclear what exactly it's doing, and when looking for where the recipe requirements get loaded it's unclear that they're being loaded there. The construction class also inherits it.

The comment's to make the line stand out more. It loads some major parts of the recipe, but is hidden at the bottom of the function.

I realise it's not ideal but i did have some trouble finding how components got into recipes.

EDIT: I've changed the comment to be more helpful, but is there a better way to make it explicit that load() comes from requirements? Something like requirements::load(recipe) perhaps (I'm not sure how you do that in C++).

[yobbobanana]

Hmm, how do i make github not hide this? All i've changed is the comment. I agree with the criticism, just can't see how better to make it explicit what's being loaded.

[BevapDin]

That method isn't actually part of the recipe class, it's part of the requirements class, which is inherited by recipe.

And therefor the function is implicitly part of the recipe class as are all the members of the requirements class.

The comment's to make the line stand out more. It loads some major parts of the recipe, but is hidden at the bottom of the function.

Than put // ############################ there, that stands out. But a comment that actually repeats what the code already tells (word by word!) is pointless and actually dangerous as it can easily become outdated. You can write rec->requirements::load(jsobj);, (here) it's the same as rec->load(jsobj).

The new comment is basically the same as the documentation of that function in requirements.h, and it does not include the fact that difficulty, time and batch time factors are loaded there, too (also missing in the functions documentation). That's exactly what I meant with "becomes outdated". When someone extends the load function, where (if at all) will they add the proper documentation? Certainly not where the function is called.

And just to clarify: I'm not opposed to comments there (or anywhere), but they should be useful and therefor contain information about how or why it's done. This PR is fine, the changes are very reasonable, I just can't stand pointless comments.

Btw, if you put stuff like the loop to "add to reverse requirements lookup for each component" into a function, you can name that function aptly (e.g. "add_reverse_lookup") and avoid the comment all together (the function name descries what is done).

---

In the spirit of inheritance, it should probably work like this (requires some overhaul, no priority):

• recipe has a default constructor without parameters, but it has a load function.
• recipe::load loads the recipe specific stuff and calls requirements::load.
• loading the recipe is now auto rec = new recipe(); rec->load(jsobj);.
• than the loaded recipe is put into the recipe map.

Alternatively, one could remove the inheritance and make the requirements a member of the recipe / construction classes. Than it would be loaded as rec->requirements_data.load(jsobj); (or whatever the member is named). Actually this seems better than inheriting from requirements. Recipes and constructions have requirements, but they are not some type of requirements.

---

Unrelated note: storing the recipe pointer in the reverse lookup map when loading it, is potential unsafe. The recipe might be re-defined by a mod, the old recipe is deleted than and the pointer you have stored becomes invalid (see check_recipe_ident).

[yobbobanana]

I'm fine with updating things so that requirements is a member in stead of being inherited. That would remove the confusion. How does it sound if I rename the requirements class to something like requirements_data, then recipes and constructions can have requirements?

Re: check_recipe_ident… Ugh. It says "check", not clobber. Even if i used something other than pointers the lookup would be wrong if it wasn't updated there. Thanks for pointing that out.

Re: reverse requirements map construction, I could put it in a separate function, I just figured the inline loop + comment was more concise. The comment's so you don't have to read the loop, but if you want to it's right there. Makes it easier to skim.

[BevapDin]

How does it sound if I rename the requirements class to something like requirements_data

Fun fact: you can use the same name for variable and class, requirements requirements; is completely valid, but can be confusing. requirements_data sounds fine, maybe singular requirement_data.

check_recipe_ident … Ugh. It says "check", not clobber. Even if i used something other than pointers the lookup would be wrong if it wasn't updated there. Thanks for pointing that out.

Feel free to change the function name to something better. Another thing I just remembered: the item blacklist feature removes items from recipes, it will also delete the recipe when it is not achievable anymore (Item_factory::finialize_item_blacklist).

The comment's so you don't have to read the loop

The function name's so I don't have to read the functions body (-:

[yobbobanana]

Aargh! Guess I should probably refactor things a little more so that either pointers aren't getting invalidated all over the place, or the standard way of dealing with recipes is by ident or id. The current lookup by ident iterates over every recipe. But then check_recipe_ident reuses both ident and id to mean something different anyway. Aargh. A clean solution for this seems tough. I'll look into it later in the week.

I ended up moving that loop to its own function :P

[yobbobanana]

I updated it to only show the recipe list if there are a dozen or less known using the item. Otherwise it displays a line like "you know many things you can make with this".

After playing with it a bit the slowdown doesn't seem severe. It's about par with the general speed of walking around. Still it would be good if someone could test it with a high level character and a large stash to see if it causes problems.

I also didn't realize how much i wanted this until i had it. It's super useful for looting, and also for crafting things that you can use to craft other things.

If it ends up being too slow, I could just remove the "can I make it?" check, but it would lose some good utility.

There's still the issue BevapDin brought up above with the recipe requirements loading method name. Not sure how to make github unhide that.

[yobbobanana]

Added a destructor to recipes which removes them from the reverse lookup when recipes get deleted (which apparently happens all over the place).

Also shuffled things around a little so that recipe requirements are no longer a superclass of recipes, but in stead a member. So now recipes and constructions have requirements in stead of being requirements, which was weird.

I think this is pretty much ready, unless there are any other comments or criticisms. Still needs testing for speed. I playtested a bit and it seems fine here.

[yobbobanana]

Ready, i think.

[MattAmmann]

I love this! Thanks for the enhancement. Here's a screenshot of this PR in action:

[moist-zombie]

Fantastic, fantastic stuff :) I'm technically on a "Cataclysm Strike" for now, since I burned myself out pretty heavily last round ;) Haven't played since a little after the 'classic zombie' expansion, though I check the 'pulse' here every two or three days :> All karma and respect to ye faithful zombie custodians!

[yobbobanana]

glad y'all like it :)

[kevingranade]

Just needs some more optimization, I'm wondering if we should have a
crafting inventory cache attached to the player.

[yobbobanana]

I thought about caching it, but it seems like there are a lot of situations in which it could change – any effect that could modify the items available to the player would invalidate it.

Although come to think of it, is there anything that could change available items without taking time? Perhaps if the current time hasn't changed then the same crafting inventory can be reused?

[kevingranade]

Movement can, it can take less than a turn.
Unfortunately it would be a mess of invalidation, but it's also one of the
worst latency operations we have, so solving it would help a lot of things.
Short of that, making the check for 'can I make this NOW' triggered by a
keypress would keep it from bothering people who aren't actively using it.

[yobbobanana]

Hmm, i assume it's easy enough to check if the player has used their moves as well. So if the player's moves left is the same, and the game turn is the same, then it could be assumed that no items have been modified, so long as anything which does modify items without taking moves or turns is considered special enough to invalidate the cached inventory itself.

In general i'm against adding more button presses if i can help it, so i'll take a look at how far i can get with caching.