Make item crafting options include recipes from nearby books #37661

wapcaplet · 2020-02-03T03:18:18Z

Summary

SUMMARY: Bugfixes "Make item description consistent with crafting UI for recipe list"

Purpose of change

Resolves #37643

Describe the solution

Instead of showing only memorized recipes at the bottom of the item description panel, show all available recipes (including those from nearby books), so the "You could use it to craft: ..." message is consistent with the recipes found in the main crafting UI for that item.

If no recipes are available (in memorized recipes or nearby books), include this at the bottom of the item description: "You know of nothing you could craft with it."

Describe alternatives you've considered

Considered preserving the original behavior in the item description of only showing learned/memorized recipes, but was persuaded by other players/developers that this was bad and should be changed.

Testing

I tested a variety of ad-hoc use cases, but here's a specific example:

Create a new world and blank 0-skill character (survivor scenario)
Use the debugging menu to spawn in:
- "What's a Transistor?"
- 1 heating element
- 1 amplifier circuit
Read the book or use debug to get Electronics skill to level 2

Then compare:

With "What's a Transistor?" nearby:
- Inspect heating element: You could use it to craft: hotplate and coffeemaker
- Go to crafting menu and / to search, c:heating element: Matches hotplate and coffemaker
- Inspect amplifier circuit, and compare recipes with crafting menu: All 7 recipes match
Without "What's a Transistor?" nearby:
- amplifier circuit only has "noise emitter (off)" in both screens
- heating element has no recipes available in either screen

Additional context

Screenshots below are cut/pasted to show item description and crafting UI for comparison.

With "What's a Transistor?" nearby, the heating element has two recipes available in both the item description and crafting menu:

Similarly, the amplifier circuit item description and crafting recipes are matching:

With the book too far away, the only recipe I get for the amplifier circuit is the level-0-auto-learned "noise emitter (off)":

And for the heating element, without "What's a Transistor?", there are no known recipes:

To encourage player to memorize more recipes, and/or make it clearer that "You could craft" would appear in this place if more recipes were known. Resolves CleverRaven#37643 Splits the existing `if` condition into two parts, first to check if known recipes are empty, and display this message at the bottom of the item description panel: You don't know anything you could craft with it. Note, unless recipes are listed, this message will be shown for all items, whether or not recipes exist for the item.

wapcaplet · 2020-02-03T04:29:44Z

I see this change caused a couple failing test cases in tests/iteminfo_test.cpp.

Marking this PR as WIP until I can fix those (I've been looking for an excuse to get into the tests anyway).

Some failed test cases in `tests/iteminfo_test.cpp` made me realize my previous commit should have kept the `DESCRIPTION_APPLICABLE_RECIPES` check, since that is a flag indicating whether the section is applicable to that kind of item, and should be shown at all. Doing that check first made it clear that the preceding block, initializing `tid` and `known_recipes`, could move inside the `if` and avoid being needlessly calculated in some cases. Due to indentation, this looks like a larger diff than it really is, but it's all the same logic as before - if no recipes are known, the section will display "You don't know anything you could craft with it." Verified that all tests in `make check` are passing now, and confirmed in-game that messages are shown as expected.

wapcaplet · 2020-02-04T00:06:47Z

A misunderstanding on my part regarding the DESCRIPTION_APPLICABLE_RECIPES flag caused the unit test failure. I have fixed it, and done a little refactoring as well. Full details in the commit message: f45a1e7

I'm satisfied with how this works, but realize it's a far-reaching change, affecting probably a majority of item descriptions, and ultimately adds very little useful information. Now that I've done it, I am not so sure that it should be accepted and merged. It's kind of a waste of text.

So I'm submitting it for consideration and discussion, suggestions of alternatives, criticism, outright ridicule etc.

Pros:

Tells player the reason there are no crafts available (lack of knowledge)
Suggests itself as a placeholder, a kind of implicit "This is where you would see craftables if you knew any"

Cons:

Emphasizes an existing discrepancy in recipe display between item info (which only includes memorized recipes) and crafting menu (which may also include non-memorized recipes from nearby books) - that is the concern described in issue "You could use it to craft" list is empty when it shouldn't #37643
Redundant and tedious - many (if not most) items will always show this message, forever

codemime · 2020-02-04T01:14:48Z

So I'm submitting it for consideration and discussion, suggestions of alternatives, criticism, outright ridicule etc.

Please see the corresponding issue #37643 (comment). In short, I think the description should match the information available through the crafting menu.

@codemime

In discussion with @codemime, determined that `player::get_available_recipes` is suitable for use here in place of `get_learned_recipes`, but my C++fu is weak and I've somehow caused a game crash that doesn't even include this file in the backtrace. Committing to ask for help on what I'm doing wrong.

wapcaplet · 2020-02-07T00:39:54Z

@codemime In commit 3228bad I tried to use get_available_recipes as we discussed, passing crafting_inv = g->u.crafting_inventory() but I must have done something horribly wrong because now every time I try to use the main crafting menu (which needs item descriptions) or inspect an item that has one or more possible recipes, the game simply crashes with a segmentation fault. The relevant part of the crash log (before getting into low-level string stuff) is all about item info:

    /usr/include/c++/7/bits/hashtable.h:1439
    Item_factory::find_template(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const
    …/src/item_factory.cpp:1216
    item::item(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, time_point, item::default_charges_tag)
    ??:?
    recipe::create_result() const
    …/src/recipe.cpp:403
    recipe::get_component_filter(recipe_filter_flags) const
    …/src/recipe.cpp:562
    item::final_info(std::vector<iteminfo, std::allocator<iteminfo> >&, iteminfo_query const*, int, bool) const
    ??:?
    item::info[abi:cxx11](std::vector<iteminfo, std::allocator<iteminfo> >&, iteminfo_query const*, int) const

I can post the full log if it'll help, but I'm betting you could look at my code and immediately know what the problem is. It's been something like 20 years since I did any C++ so my syntax may be awkward...

Also, let's just take a moment to appreciate that commit id: 3228bad.

src/item.cpp

Avoid ending up with a null pointer refererence from `of_component`. Shorten the "no recipes" message slightly to make it fit without wrapping more often.

codemime · 2020-02-07T01:41:29Z

Also, let's just take a moment to appreciate that commit id: 3228bad.

Yeah, I suspect that's Linus making fun of our code )). This just gave me an idea. It would be cool to make a generator of commits with funny ids. Some say, SHA-1 was cracked, so there can be something better than the brute-force approach for this.

wapcaplet · 2020-02-07T01:46:30Z

I've adjusted the PR summary and description to reflect the new nature of this fix, and added screenshots confirming the consistent behavior I am seeing now between the two UIs.

src/item.cpp

wapcaplet · 2020-02-08T22:28:50Z

Whoops, hold on - that test case isn't working yet. Back in WIP

Test coverage for modified item info behavior

wapcaplet · 2020-02-09T01:06:29Z

The test cases are in much better shape now, but I'm still getting semi-random failures in a couple of them; for examine sometimes the crowbar will show up with a bunch of higher-level recipes (foldable-light frame, wheelchair wheels, pipe bomb, nail rifle etc.) instead of the expected level-0 "makeshift crowbar" only.

Seems like something is bleeding over from other tests; probably I need to clean up / reset the global user g->u - I'm already using empty_skills() and empty_traits() but maybe it takes more than that...

Reduced to essential assertions; all passing now

wapcaplet · 2020-02-09T01:24:30Z

All tests are passing now. Assuming this passes build checks, it should be ready to merge.

Caught by Travis CI

wapcaplet mentioned this pull request Feb 3, 2020

"You could use it to craft" list is empty when it shouldn't #37643

Closed

wapcaplet changed the title ~~Add item description when lacking crafting knowledge~~ WIP Add item description when lacking crafting knowledge Feb 3, 2020

wapcaplet added 2 commits February 5, 2020 19:25

Merge branch 'master' into you-dont-know-recipes

f2d36e9

codemime reviewed Feb 7, 2020

View reviewed changes

src/item.cpp Outdated Show resolved Hide resolved

Fix crash, tweak message

5617811

Avoid ending up with a null pointer refererence from `of_component`. Shorten the "no recipes" message slightly to make it fit without wrapping more often.

wapcaplet changed the title ~~WIP Add item description when lacking crafting knowledge~~ Make item crafting options include recipes from nearby books Feb 7, 2020

WIP Add test for item recipes in description

793e3b5

codemime reviewed Feb 8, 2020

View reviewed changes

src/item.cpp Outdated Show resolved Hide resolved

wapcaplet added 2 commits February 8, 2020 13:53

Merge branch 'master' into you-dont-know-recipes

780e969

Use ternary expression instead of 6-liner

3ffc4f0

codemime self-assigned this Feb 8, 2020

wapcaplet changed the title ~~Make item crafting options include recipes from nearby books~~ WIP Make item crafting options include recipes from nearby books Feb 8, 2020

WIP Improve item recipe tests (still failing)

3561ef3

Test coverage for modified item info behavior

Fix item recipe info test cases

6fb8d13

Reduced to essential assertions; all passing now

wapcaplet changed the title ~~WIP Make item crafting options include recipes from nearby books~~ Make item crafting options include recipes from nearby books Feb 9, 2020

weekly-digest bot mentioned this pull request Feb 9, 2020

Weekly Digest (2 February, 2020 - 9 February, 2020) #37848

Closed

Remove unused crowbar variable

39d32cf

Caught by Travis CI

weekly-digest bot mentioned this pull request Feb 9, 2020

Weekly Digest (2 February, 2020 - 9 February, 2020) #37851

Closed

codemime merged commit 39d32cf into CleverRaven:master Feb 9, 2020

This was referenced Feb 16, 2020

Weekly Digest (9 February, 2020 - 16 February, 2020) #38055

Closed

Weekly Digest (9 February, 2020 - 16 February, 2020) #38058

Closed

wapcaplet deleted the you-dont-know-recipes branch March 8, 2020 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make item crafting options include recipes from nearby books #37661

Make item crafting options include recipes from nearby books #37661

wapcaplet commented Feb 3, 2020 •

edited

Loading

wapcaplet commented Feb 3, 2020

wapcaplet commented Feb 4, 2020 •

edited

Loading

codemime commented Feb 4, 2020

wapcaplet commented Feb 7, 2020

codemime commented Feb 7, 2020

wapcaplet commented Feb 7, 2020

wapcaplet commented Feb 8, 2020

wapcaplet commented Feb 9, 2020

wapcaplet commented Feb 9, 2020

Make item crafting options include recipes from nearby books #37661

Make item crafting options include recipes from nearby books #37661

Conversation

wapcaplet commented Feb 3, 2020 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

wapcaplet commented Feb 3, 2020

wapcaplet commented Feb 4, 2020 • edited Loading

codemime commented Feb 4, 2020

wapcaplet commented Feb 7, 2020

codemime commented Feb 7, 2020

wapcaplet commented Feb 7, 2020

wapcaplet commented Feb 8, 2020

wapcaplet commented Feb 9, 2020

wapcaplet commented Feb 9, 2020

wapcaplet commented Feb 3, 2020 •

edited

Loading

wapcaplet commented Feb 4, 2020 •

edited

Loading