search

minetest-mods / craftguide

:book: The most comprehensive Crafting Guide on Minetest
Other
43 stars 20 forks source link

Progressive: refactor, fix some bugs, add API function #46

Closed p-ouellette closed 5 years ago

p-ouellette commented 5 years ago

See this comment: https://github.com/minetest/minetest_game/issues/1435#issuecomment-267147674 by @Wuzzy2 for some good reasons for this. I can add an option for the previous behavior (only show recipes if you have all the items) if requested.

init_data, reset_data, and get_init_items were moved. I separated search filtering and progressive mode filtering (searching uses filter_items and progressive uses get_progressive_items).

This also fixes several bugs in the previous progressive mode.

kilbith commented 5 years ago

I take the fixes but I’m opposed to changing the current system.

p-ouellette commented 5 years ago

Why?

Current system: It's nearly impossible to discover all crafts as you have to by chance have all the required items in your inventory. Maybe you have all the required items for a recipe, but some of them are in a chest. You don't find out about the recipe. The result is players spending a lot of time switching stuff in and out of their inventory trying to find that perfect combination.

Proposed system: player discovers a new item and places it in their inventory. They now see all the new recipes they have access to instead of having to place different combinations of every other item they have discovered into their inventory until they find new recipes.

And as Wuzzy said:

It also adds to gameplay IMO: Whenever the player obtains a new item, it can be interesting or even exciting to see what new stuff can be crafted from it.

I can make the current system the default with an option for the new one if you want.

Wuzzy2 commented 5 years ago

While I like the idea of progressive mode, it also has a serious downside: You always need to carry certain items in order to see crafts. If your inventory is empty, you don't even see torch.

Progressive mode needs to be re-thought before it becomes default. IMHO.

kilbith commented 5 years ago

I agree the current system needs an overhaul but this PR is not the right approach for me. A better one would be to remember the already discovered recipes and to save them in meta.

And showing the whole recipe with only one craft item in inventory does not sound right at all.

p-ouellette commented 5 years ago

I wouldn't say that not being able to see crafts if you have an empty inventory is bad. You can't craft anything if your inventory is empty, so why do you need to see recipes? Also wood is usually the first item one collects in a game. One you have wood, you can see the recipe for planks, then sticks, then torches. After that you should be able to remember that you need a stick to craft a torch.

Note that this PR doesn't make progressive mode the default.

p-ouellette commented 5 years ago

As for saving discovered recipes, when is a recipe "discovered"? If it's when you have all the required ingredients in your inventory, how does that solve the issues with the current system?

What's wrong with showing recipes when you have only one of the required items? Most recipes have only 1 or 2 different input items. If you can see the missing items, you know what to look for.

Napiophelios commented 5 years ago

I would considered "discovered" as a recipe that has been suggested and then used to actually craft something.

p-ouellette commented 5 years ago

I restored the current behavior for now. This PR is just a refactor and bugfix now, plus an API function that allows mods to define which recipes get shown in progressive mode. A mod could use this to implement the suggested discovery system.

bell07 commented 5 years ago

In case I have an unknown item in inventory I get a crash

in callback on_playerReceiveFields(): ...kte/minetest/bin/../mods/craftguide/init.lua:530: attempt to index a nil value
2019-01-12 14:47:48: ERROR[Main]: stack traceback:
2019-01-12 14:47:48: ERROR[Main]:   ...kte/minetest/bin/../mods/craftguide/init.lua:530: in function 'progressive_show_recipe'
2019-01-12 14:47:48: ERROR[Main]:   ...kte/minetest/bin/../mods/craftguide/init.lua:560: in function 'progressive_filter_recipes'
2019-01-12 14:47:48: ERROR[Main]:   ...kte/minetest/bin/../mods/craftguide/init.lua:573: in function 'get_progressive_items'
2019-01-12 14:47:48: ERROR[Main]:   ...kte/minetest/bin/../mods/craftguide/init.lua:584: in function 'init_data'
2019-01-12 14:47:48: ERROR[Main]:   ...kte/minetest/bin/../mods/craftguide/init.lua:755: in function 'on_enter'
2019-01-12 14:47:48: ERROR[Main]:   ...e/minetest/bin/../games/minetest_game/mods/sfinv/api.lua:133: in function 'set_page'
p-ouellette commented 5 years ago

Thanks, fixed

kilbith commented 5 years ago

Merged. Thanks for your contribution.

https://github.com/minetest-mods/craftguide/commit/50317cc20eee3d04805f03f8f3a9532fc1fd0a3e