minetest-mods / craftguide

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

Fix missed items in cache #116

Closed bell07 closed 4 years ago

bell07 commented 4 years ago

Have some times the issue in Whynot that simple recipes like for default:stick are missed in guilde. Now found the reason: The table_merge usage was wrong at this place.

kilbith commented 4 years ago

Your code wasn't correct but try out this patch: https://github.com/minetest-mods/craftguide/commit/c0823faad34501f48543bb565fbd1b15a91af08f

bell07 commented 4 years ago

Tested it, thank you!. Now the issue is multiple recipes are loaded up to 3 times. Most are twice. Previously you had a function to check for recipe duplicates, now there is no such check.

Just for reference, in smart_inventory I used the recipe reference as unique key for recipe. recipe_cache[item][recipe] = enhanced_recipe_data Maybe you give this way a try.

kilbith commented 4 years ago

Now the issue is multiple recipes are loaded up to 3 times. Most are twice.

Yeah I noticed that... Can you tell me which recipes were not showing up prior to this patch?

kilbith commented 4 years ago

This time it's really fixed: https://github.com/minetest-mods/craftguide/commit/bfdb67783b31936ab94c93e9746aa977e0d60eb4

Is there anything wrong with aliased items?

bell07 commented 4 years ago

Now the recipe group:wood -> default:stick (for example) is lost again in Whynot game. Like before the both changes. In Minetest game the recipe exists, so I have no idea what is wrong. Maybe you can test with the Whynot Game game? The craftguide in game is up2date.

bell07 commented 4 years ago

I think in Whynot there is an issue because homedecor registers some other recipes for default:stick. I think the issue is related to core.register_craft redefinition. If the redefined method does register recipes, all recipes for the same item registered before trough the original function are hidden. because recipes_cache[item] already exists.

EDIT: I think the core.register_craft should fill the recipes_cache but store additional data to own lua-table. Then the cache_recipes() should check all recipes for the additional data from the lua-table.

EDIT2: Commented the core.register_craft redefinition out and the missed recipe is back!

EDIT3: Please check the next proposal: Line 1506: Change to if def.type ~= "fuel" and def.type ~= "shapeless" and def.type ~= nil then The shapeless and default recipes should not be added to the recipes_cache at this place because handled by get_init_items / resolve_aliases trough cache_recipes() Works fine for me, no dups, all recipes (seems to be) there

kilbith commented 4 years ago

https://github.com/minetest-mods/craftguide/commit/84a7377ed0e179d30045ebd1954f299a3c1637fd

I hope it doesn't skip another recipes.

... and I guess this was a useless code now: https://github.com/minetest-mods/craftguide/commit/bba7dd81b962e320b405506976f67f83c4df1588

Please test this, I have the strong feeling to be walking on eggs.

bell07 commented 4 years ago

Thanks, now it looks fine! Theoretically the last piece of the issue remains with cooking recipes. In case a cooking recipe added by redefined core.register_craft(), the "normal" recipes for the same output item will be hidden, because recipes_cache[name] already exists. Maybe you build up new cooking_cache table with cooktimes instead of recipes_cache usage, like you do it with burntimes for fuel in fuel_cache?

kilbith commented 4 years ago

Please test this commit: https://github.com/minetest-mods/craftguide/commit/89979a86100c8b1cc6c793ae296efe6bf6ff16c8 Also this: https://github.com/minetest-mods/craftguide/commit/fa1e330856aaea3f5d69d0553ecee5d7b9ed2be2

Would be cool if we had some kind of unittests in craftguide.

bell07 commented 4 years ago

The code changes looks right, but the most burning and cooking recipes are away for now. :-( For unit test I think we need a test-mod that register some recipes that should be found in inventory..

kilbith commented 4 years ago

Shit... tell me which ones.

bell07 commented 4 years ago

The basics. I miss the information the wood or tree is burneable, or that gold can be cooked from gold lump.

kilbith commented 4 years ago

Fuel fixed, cooking still broken: https://github.com/minetest-mods/craftguide/commit/0b37b2bb2fc154bc1d207870d48f9242ce546cb0

kilbith commented 4 years ago

Finally fixed but must be checked: https://github.com/minetest-mods/craftguide/commit/d57cb7865b642ff9147a9040c2883270a2bd3b9e

bell07 commented 4 years ago

Still the most burning and cooking recipes are not in guide :-( I look for tree burning and gold smelting

bell07 commented 4 years ago

Still the most burning and cooking recipes are not in guide :-( I look for tree burning and gold smelting in whynot game. Tested with mintest_game, recipes are there. I think the timing is different if default mod is loaded before or after the craftguide..

kilbith commented 4 years ago

Cooking recipes fixed: https://github.com/minetest-mods/craftguide/commit/7ea6899b061f6eefcb87862d3464da113b83dbd8

kilbith commented 4 years ago

Fuel recipes fixed: https://github.com/minetest-mods/craftguide/commit/74785bca5e1a792f5277b853ad7d48de438dd239

bell07 commented 4 years ago

Finally! I tested and was able to find all the missed recipes! By the way, I created and tested in addition with unit test I wrote for: https://github.com/bell07/craftguide/commit/7cb6a10bd304296777025375def468ef8f454b9e If you like it, you can add it to upstream. For testing just uncomment the first and the last line, then search for the 4 items and check if all 8 recipes are found. With current version I found all of them ;-) The split to 2 files are because some recipes are registered before the mod is loaded and core.register_craft is redefined.

kilbith commented 4 years ago

Thanks, but this is far from what I expect from the unittests. It should handle hundreds of weird combinations including custom recipes (drops, etc.), malformed recipes, aliased stuff and the like. Also these recipes should be processed through the formspec rendering functions without opening the craftguide.

kilbith commented 4 years ago

By the way, I just realized that I broke the replacements field which is actually present in all kinds of recipes, so I had to put back some code in core.registered_craft: https://github.com/minetest-mods/craftguide/commit/2b8dbcea49a8b2ee5c480e7c5898baa9a87e6c60

bell07 commented 4 years ago

We are at beginning. In Whynot the recipe "Wood" -> "Stick" is missed, The recipe wood is burneable is missed. Smelting recpes seems to be fine. My micro-unit-test is just for issue we try to catch. I tested and the next recipes are missed in inventory:

minetest.register_craft({
    output = "craftguide:test_1_1 2",
    recipe = {
        {"craftguide:test_1_2 2"},
    }
})

minetest.register_craft({
    type = "cooking",
    output = "craftguide:test_1_1",
    recipe = "craftguide:test_1_2",
})

Both are registered with original core.register_craft, before the register_craft is redefined by craftguide.

The next recipes are loaded by the changed core.register_craft and already in recipes_cache, so the recipes above are ignored in loop trough registered_items.

minetest.register_craft({
    output = "craftguide:test_1_1 3",
    recipe = {
        {"craftguide:test_1_2 3"},
    }
})

minetest.register_craft({
    type = "cooking",
    output = "craftguide:test_1_1",
    recipe = "craftguide:test_2_2",
})
kilbith commented 4 years ago

It seems to be working well if you comment-out these checks:

Can you confirm?

EDIT: ah no, I'm loosing another recipes.

bell07 commented 4 years ago

I expected duplicate recipes, but seems to be beter. The "micro-unit-test" got all recipes, but in whynot some burningtimes recipes are missed

kilbith commented 4 years ago

I may have achieved the final fix in this fight to the death with the code: https://github.com/minetest-mods/craftguide/commit/c30db9166282384382b2ee6e40d3324495ced971

How does that look?

bell07 commented 4 years ago

The Wood planks burning time still lost grafik Other wood types are fine: grafik

Maybe you can debug it with current whynot version? I pushed the last changes to github and contentdb.

kilbith commented 4 years ago

Are you sure to have registered the fuel recipe to wood planks?

EDIT: https://github.com/bell07/minetest-game-whynot/blob/master/mods/minetest_game/default/crafting.lua#L492

kilbith commented 4 years ago

print(get_burntime("default:wood")) returns 7 on bare minetest_game but 0 with whynot. I guess either there's something wrong in your game, or it should be solved inside the core.register_craft override (with no warranty it can handle all fuel recipes).

https://github.com/minetest-mods/craftguide/commit/74785bca5e1a792f5277b853ad7d48de438dd239

Finally! I tested and was able to find all the missed recipes!

BTW: after resetting to this commit, the wooden planks are not marked as burnable either.

EDIT: I just tried to put apple wooden planks in the furnace in WhyNot... they are not burnable. There is definitely something wrong in your game.

bell07 commented 4 years ago

Thank you for pointing that out! I didn't get the idea that burnig recipe might be broken in game ... Searching now for the reason, before next retest

bell07 commented 4 years ago

Found the reason for wood burning issue by removing mods till it does work again ... It was the mod mtg_plus :-(

minetest.register_node("mtg_plus:goldwood", {
    description = S("Goldwood"),
    _doc_items_longdesc = S("Goldwood is a precious artificial kind of wood made by enriching wood with gold. Goldwood is fireproof and notable for its bright yellowy appearance."),
    tiles = {"mtg_plus_goldwood.png"},
    is_ground_content = false,
    groups = {choppy = 2, wood = 1},
    sounds = default.node_sound_wood_defaults(),
})

minetest.clear_craft({
    type = "fuel",
    recipe = "mtg_plus:goldwood",
})

clears for any reason the default wood recipe :-(

Reported to Wuzzy in forum