Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How am I meant to test empty ItemStacks from crafting recipes? #80

Open
Montandalar opened this issue Mar 31, 2022 · 3 comments
Open

How am I meant to test empty ItemStacks from crafting recipes? #80

Montandalar opened this issue Mar 31, 2022 · 3 comments
Assignees
Labels
bug Something isn't working in-latest-docker Included with latest docker images, might not yet be available elsewhere

Comments

@Montandalar
Copy link

Hi,

So I am implementing a privileged-protected recipe system using register_craft_predict and register_on_craft. In these callbacks you get a table with 9 ItemStacks, and in my test for the empty slots in the grid I thought I would insert ItemStack(""), but the source of Mineunit says it's meant to cause an error with an empty string. I don't think my function will work without "" as the argument though. Do you know how I should proceed?

Code is here:

@S-S-X
Copy link
Owner

S-S-X commented Apr 3, 2022

I think I've just followed documentation here:

An ItemStack is a stack of items.

It can be created via ItemStack(x), where x is an ItemStack,
an itemstring, a table or nil.

Empty string however is valid with actual engine, for some reason I did leave explicit note "do not fix" there but I do not really anymore remember reasoning for that.
Not too long ago I was going to change that to allow empty string but in the end for some reason did not change it...

Reasoning could have been strict rules: "be explicit" which would mean that empty stack should be created with

stack = ItemStack(nil)

instead of

stack = ItemStack("")

Now I'm pretty sure this is a bug in Mineunit and now again I'm leaning towards fixing it, would be nice to remember exact reason why I originally decided to do that...

Also now that I tested engine behavior:

  • Minetest engine 5.0
    • Crashes with ItemStack()
    • Succeeds with ItemStack(nil)
    • Succeeds with ItemStack("")
  • Minetest engine 5.5
    • Succeeds with ItemStack()
    • Succeeds with ItemStack(nil)
    • Succeeds with ItemStack("")

So even if it was for "strict" rules it would still be bad option as ItemStack() is allowed and crashes older engine while ItemStack("") succeeds with old engine.

@S-S-X S-S-X added the bug Something isn't working label Apr 3, 2022
@S-S-X
Copy link
Owner

S-S-X commented Apr 3, 2022

For your test you can simply use ItemStack() instead of ItemStack(""), I'll leave this issue hanging here for a moment attempting to recall why I've stated "do not fix" for apparent bug.

That explicit "do not fix" note tells me that there might be some actual reason behind it, when I wrote note I already knew that I'm going to spot bug some day there so there has to be some reason why I wrote that note...

@S-S-X
Copy link
Owner

S-S-X commented Aug 7, 2022

So far I've not found any real reason for what I've written there... this should be fixed, preferably with behavior depending on engine version and warnings for usage that is not universally compatible: call without args throws warning or error.

@S-S-X S-S-X self-assigned this Jan 11, 2024
@S-S-X S-S-X added this to Mineunit Jan 11, 2024
@S-S-X S-S-X moved this to Ready in Mineunit Jan 11, 2024
@S-S-X S-S-X moved this from Ready to In progress in Mineunit Jan 11, 2024
@S-S-X S-S-X added the in-latest-docker Included with latest docker images, might not yet be available elsewhere label Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in-latest-docker Included with latest docker images, might not yet be available elsewhere
Projects
Status: In progress
Development

No branches or pull requests

2 participants