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

Itemstack details #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/mineunit.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

name: mineunit

on: [push, pull_request]
on: [push, pull_request, workflow_dispatch]

jobs:
build:
Expand Down
2 changes: 1 addition & 1 deletion init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ local default_config = {
engine_version = "mineunit",
}

for k,v in pairs(mineunit_conf_defaults) do
for k,v in pairs(mineunit_conf_defaults or {}) do
default_config[k] = v
end

Expand Down
76 changes: 58 additions & 18 deletions itemstack.lua
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function ItemStack:set_name(item_name)
assert.is_string(item_name, "ItemStack:set_name expected item_name to be string")
self._name = item_name
if item_name == "" then
self:set_count(0)
self:clear()
return true
end
return false
Expand All @@ -20,15 +20,27 @@ end
function ItemStack:get_count() return self._count end
--* `set_count(count)`: returns a boolean indicating whether the item was cleared
-- `count`: number, unsigned 16 bit integer
function ItemStack:set_count(count) self._count = count end
function ItemStack:set_count(count)
if count > 0 and count <= 65535 then
self._count = count
return true
else
self:clear()
return false
end
end
--* `get_wear()`: returns tool wear (`0`-`65535`), `0` for non-tools.
function ItemStack:get_wear() return self._wear end
--* `set_wear(wear)`: returns boolean indicating whether item was cleared
-- `wear`: number, unsigned 16 bit integer
function ItemStack:set_wear(wear)
assert(wear <= 65535, "ItemStack:set_wear invalid wear value: "..tostring(wear))
wear = wear < 0 and -((-wear) % 65536) or wear
self._wear = math.max(0, wear < 0 and 65536 + wear or wear)
if wear <= 65535 then
self._wear = wear % 65536
return true
else
self.clear()
return false
end
Comment on lines +37 to +43
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this updated engine 5.5 behavior? (yeah previous wear behavior was bit weird but kinda useful)

Just in case reminder link #7
Also for assertion link #64 as some technic mod tests depends on strict validation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this updated engine 5.5 behavior? (yeah previous wear behavior was bit weird but kinda useful)

I'm not certain, I'll take a look. If I remember, the reference I was looking at just casts the value as uint16.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is how it's been done since at least 0.4.17.1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you confusing this with "add_wear"?

Copy link
Owner

@S-S-X S-S-X Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah actually it is same behavior but just like engine allows destroying items by setting invalid wear?
Should prob just add some simplified strict mode to throw errors, probably duping assert as strict_assert or something for easy use (and custom error behavior handling)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now wondering why I've even done that as it only affects values > 65536 and those will kill stack anyway.... 🤷 well, looks way better now so just have to add assertion thing.
I think I could try to look into that a bit tomorrow, would be nice to get this merged soon anyway.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're talking about w.r.t. simplified strict mode or throwing errors?

Strict mode means that error is raised when something is done that does not seem clearly intended behavior like destroying tool stack by setting invalid value for it. That is also application specific thing as sometimes it is meant to be destroyed while sometimes not, would however like to have assertion near actual call that causes potentially unwanted behavior for this specific case.
Not sure how this case should be exactly handled, so far was handled with straight assertion probably just because that happened to be only use case I needed back then...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but destroying the stack by setting the value above 65535 is the intended behavior. You used the tool too much, now it's broken. The assertion that was there previously was wrong.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is why clarified with "That is also application specific thing as sometimes it is meant to be destroyed while sometimes not".
True that it is kind of wrong and should probably have been handled different way in tests from beginning, use case for that assertion is with indestructible rechargeable power tools.

It could have been done better with more specific assertions in test set by verifying that stack is still valid but... well, that was not done... but then early error also proved to be useful as when tool stack got accidentally destroyed it eventually caused error on completely irrelevant part of mod and that's reason why "would however like to have assertion near actual call that causes potentially unwanted behavior for this specific case"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs way to mark item def as destructible/indestructible through "side effects" and global configuration for default behavior... I think that would work best here.

end
--* `get_meta()`: returns ItemStackMetaRef. See section for more details
function ItemStack:get_meta() return self._meta end
Expand Down Expand Up @@ -63,7 +75,15 @@ function ItemStack:get_short_description()
if value then return value:gmatch("[^\r\n]+")() end
end
--* `clear()`: removes all items from the stack, making it empty.
function ItemStack:clear() self._count = 0 end
-- https://github.com/minetest/minetest/blob/0f25fa7af655b98fa401176a523f269c843d1943/src/inventory.h#L63-L69
-- https://github.com/minetest/minetest/blob/0f25fa7af655b98fa401176a523f269c843d1943/src/script/lua_api/l_item.cpp#L206-L214
function ItemStack:clear()
self._name = ""
self._count = 0
self._wear = 0
self._meta:_clear()
return true
end
--* `replace(item)`: replace the contents of this stack.
-- `item` can also be an itemstring or table.
function ItemStack:replace(item)
Expand All @@ -76,12 +96,25 @@ end
--* `to_string()`: returns the stack in itemstring form.
-- https://github.com/minetest/minetest/blob/5.4.0/src/inventory.cpp#L59-L85
function ItemStack:to_string()
-- FIXME: Does not currently serialize metadata
return ("%s %d %d"):format(
self:get_name(),
self:get_count(),
self:get_wear()
)
local name = self:get_name()
local count = self:get_count()
local wear = self:get_wear()
if name == "" or count == 0 then
return ""
else
local parts = {name}
if count > 1 or wear ~= 0 or not self._meta:_empty() then
table.insert(parts, tostring(count))
end
if wear ~= 0 or not self._meta:_empty() then
table.insert(parts, tostring(wear))
end
if not self._meta:_empty() then
table.insert(parts, self._meta:_serialize())
end

return table.concat(parts, " ")
end
end
--* `to_table()`: returns the stack in Lua table form.
function ItemStack:to_table()
Expand Down Expand Up @@ -135,6 +168,14 @@ function ItemStack:add_item(item)
return leftover
end

if self:get_name() ~= leftover:get_name() then
return leftover
end

if self._meta ~= leftover._meta then
return leftover
end

local stack_max = item:get_stack_max()
local count = self:get_count()
local space = stack_max - count
Expand All @@ -156,7 +197,7 @@ function ItemStack:item_fits(item)
return true
end
local stack = ItemStack(item)
if self:get_name() == stack:get_name() then
if self:get_name() == stack:get_name() and self._meta == item._meta then
return self:get_free_space() >= stack:get_count()
end
return false
Expand All @@ -165,11 +206,11 @@ end
-- Take (and remove) up to `n` items from this stack
-- `n`: number, default: `1`
function ItemStack:take_item(n)
local taken = ItemStack(self)
n = math.min(self:get_count(), n or 1)
self:set_count(self:get_count() - n)
local stack = ItemStack(self)
stack:set_count(n)
return stack
taken:set_count(n)
return taken
end
--* `peek_item(n)`: returns taken `ItemStack`
-- Copy (don't remove) up to `n` items from this stack
Expand All @@ -183,8 +224,7 @@ function ItemStack:peek_item(n)
end

function ItemStack:__tostring()
local count = self:get_count()
return 'ItemStack("' .. self:get_name() .. (count > 1 and " "..count or "") .. '")'
return 'ItemStack("' .. self:to_string() .. '")'
end

-- TODO: Allows `same` assertions but in corner cases makes mod code to return true where engine would return false.
Expand Down
89 changes: 88 additions & 1 deletion metadata.lua
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

mineunit("common/misc_helpers")
local json = require('mineunit.lib.json')

local function assert_invlist_index(index, size)
assert(type(index) == "number" and math.floor(index) == index, "InvList:set_stack: Invalid InvList stack index")
Expand Down Expand Up @@ -304,7 +305,9 @@ function MetaDataRef:from_table(t)
self._data = table.copy(t.fields)
end
end
function MetaDataRef:equals(other) error("NOT IMPLEMENTED") end
function MetaDataRef:equals(other)
return self:__eq(other)
end

-- TODO: Allows `same` assertions but in corner cases makes mod code to return true where engine would return false.
-- Requires either overriding luassert `same` (nice for users) or only allowing special assertions (not so nice).
Expand All @@ -322,6 +325,86 @@ function MetaDataRef:__eq(other)
return false
end

function MetaDataRef:_empty()
for _ in pairs(self._data) do
return false
end
return true
end

function MetaDataRef:_clear()
for key in pairs(self._data) do
self._data[key] = nil
end
end

--[[
serialize metadata, for use in itemstrings
https://github.com/minetest/minetest/blob/0f25fa7af655b98fa401176a523f269c843d1943/src/itemstackmetadata.cpp#L61-L71
]]
local DESERIALIZE_START = "\01"
local DESERIALIZE_KV_DELIM = "\02"
local DESERIALIZE_PAIR_DELIM = "\03"
function MetaDataRef:_serialize()
local parts = {}
table.insert(parts, DESERIALIZE_START)
for k, v in pairs(self._data) do
if k ~= "" or v ~= "" then
table.insert(parts, k)
table.insert(parts, DESERIALIZE_KV_DELIM)
table.insert(parts, v)
table.insert(parts, DESERIALIZE_PAIR_DELIM)
end
end

return json.encode(table.concat(parts, ""))
end

--[[
deserialize metadata, for use in itemstrings
https://github.com/minetest/minetest/blob/0f25fa7af655b98fa401176a523f269c843d1943/src/itemstackmetadata.cpp#L73-L94
]]
function MetaDataRef:_deserialize(s)
local ds = json.decode(s)
self:_clear()

local function find_next(i)
if i > #ds then
return
end
local key_start = i
while ds:sub(i, i) ~= DESERIALIZE_KV_DELIM and i <= #ds do
i = i + 1
end
local key_end = i - 1
i = i + 1
local value_start = i
while ds:sub(i, i) ~= DESERIALIZE_PAIR_DELIM and i <= #ds do
i = i + 1
end
local value_end = i - 1
i = i + 1
return i, ds:sub(key_start, key_end), ds:sub(value_start, value_end)
end

if ds:sub(1, 1) == DESERIALIZE_START then
local key, value
local i = 2
while true do
i, key, value = find_next(i)
if i and key and value then
self._data[key] = value
else
break
end
end
else
error(("error deserializing %q (%q)"):format(s, ds))
-- "BACKWARDS COMPATIBILITY"
self._data[""] = s
end
end

mineunit.export_object(MetaDataRef, {
name = "MetaDataRef",
constructor = function(self, value)
Expand All @@ -330,6 +413,10 @@ mineunit.export_object(MetaDataRef, {
obj = {}
elseif mineunit.utils.type(value) == "MetaDataRef" then
obj = table.copy(value)
elseif type(value) == "string" then
local it = MetaDataRef()
it:_deserialize(value)
return it
else
print(value)
error("TYPE NOT IMPLEMENTED: " .. type(value))
Expand Down
7 changes: 6 additions & 1 deletion player.lua
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,12 @@ local function swap_stack(toinv, tolist, toindex, frominv, fromlist, fromindex,
end

-- Place source stack into target inventory
local placedstack = count ~= -1 and stack:take_item(count) or ItemStack(stack)
local placedstack
if count == -1 then
placedstack = ItemStack(stack)
else
placedstack = stack:take_item(count)
end
toinv:set_stack(tolist, toindex, placedstack)

-- Return leftovers to source inventory
Expand Down
46 changes: 46 additions & 0 deletions spec/itemstack_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,50 @@ describe("ItemStack", function()

end)

describe("stack metadata", function()

it("metadata serializes in itemstring", function()
local stack = ItemStack("test")
local stack_meta = stack:get_meta()
stack_meta:set_int("foo", 1)
assert.equals("test 1 0 \"\\u0001foo\\u00021\\u0003\"", stack:to_string())
end)

it("metadata properly initialized by itemstring", function()
local stack = ItemStack("test 1 0 \"\\u0001foo\\u00021\\u0003\"")
local stack_meta = stack:get_meta()
assert.equals("", stack_meta:get_string(""))
assert.equals(1, stack_meta:get_int("foo"))
end)

it("allow stacking matched metadata", function()
local stack1 = ItemStack("test 1 0 \"\\u0001foo\\u00021\\u0003\"")
local stack2 = ItemStack("test 1 0 \"\\u0001foo\\u00021\\u0003\"")

assert.is_true(stack1:item_fits(stack2))

local remainder = stack1:add_item(stack2)

assert.is_true(remainder:is_empty())
assert.equals(2, stack1:get_count())
local meta = stack1:get_meta()
assert.equals(1, meta:get_int("foo"))
end)

it("disallow stacking mismatched metadata", function()
local stack1 = ItemStack("test 1 0 \"\\u0001foo\\u00021\\u0003\"")
local stack2 = ItemStack("test 1 0 \"\\u0001foo\\u00022\\u0003\"")

assert.is_false(stack1:item_fits(stack2))

local remainder = stack1:add_item(stack2)

assert.equals("test 1 0 \"\\u0001foo\\u00022\\u0003\"", remainder:to_string())
assert.equals(1, stack1:get_count())
local meta = remainder:get_meta()
assert.equals(2, meta:get_int("foo"))
end)

end)

end)
9 changes: 6 additions & 3 deletions spec/player_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,8 @@ describe("Mineunit Player", function()
end)

it("works without source index", function()
SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 1)
local can_put_count = SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 1)
assert.same(can_put_count, 33)
local expected = InvRef()
expected:set_size("chest", 3)
expected:set_stack("chest", 1, ItemStack("stone 33"))
Expand All @@ -386,7 +387,8 @@ describe("Mineunit Player", function()
end)

it("works with source index", function()
SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 2, 2)
local can_put_count = SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 2, 2)
assert.same(can_put_count, 42)
local expected = InvRef()
expected:set_size("chest", 3)
expected:set_stack("chest", 1, ItemStack("stone 33"))
Expand All @@ -396,7 +398,8 @@ describe("Mineunit Player", function()
end)

it("works with ItemStack", function()
SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 3, ItemStack("stone 96"))
local can_put_count = SX:do_metadata_inventory_put({x=0,y=1,z=0}, "chest", 3, ItemStack("stone 96"))
assert.same(can_put_count, 96)
local expected = InvRef()
expected:set_size("chest", 3)
expected:set_stack("chest", 1, ItemStack("stone 33"))
Expand Down