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

Try to preserve metatable when exchanging data with the async env #14369

Merged
merged 12 commits into from Mar 6, 2024

Conversation

y5nw
Copy link
Contributor

@y5nw y5nw commented Feb 14, 2024

Add compact, short information about your PR for easier understanding:

This PR is similar to #14360 in terms of the goal (trying to preserve metatable information), but this one is implemented differently (and in a simpler way, as certain cases in minetest.serialize are irrelevant here). The two PRs are otherwise not related to each other.

  • Goal of the PR
    Preserve metatable information when passing data to/from the async environment
  • How does the PR work?
    It keeps a mapping between certain metatables and a name, the latter of which is kept with the data when transferred to a different environment and then used to set the metatable on the reconstructed data.
    Note that, unlike Try to preserve metatable information in minetest.serialize #14360, this PR only handles tables. It does not handle userdata objects as these are handled by C++ code directly.
  • Does it resolve any reported issue?
    Fixes Vector metatable doesn't carry over to async environment #13644.
  • Does this relate to a goal in the roadmap?
    2. Bugfix; the issue linked above is labeled as a bug.

To do

This PR is Ready for Review.

  • Metatables are preserved.
  • Add documentation.

How to test

The base test case for vectors should be covered by the unittests in the Devtest.

To test the modding API, use the following mod:

-- init.lua
local MP = minetest.get_modpath(minetest.get_current_modname())

dofile(MP .. "/pair.lua")
minetest.register_async_dofile(MP .. "/pair.lua")

local list = pair(1, pair(2, nil))
minetest.handle_async(function(x)
	return x
end, function(lst)
	print(list, lst, list == lst)
end, list)
-- pair.lua
local mt = {
	__eq = function(x, y)
		return x[1] == y[1] and x[2] == y[2]
	end,
	__tostring = function(x)
		return ("(%s . %s)"):format(x[1], x[2])
	end,
}

function pair(x, y)
	return setmetatable({x, y}, mt)
end

minetest.register_async_metatable("async_test:pair", mt)

The above mod should print a line to the console:

(1 . (2 . nil)) (1 . (2 . nil)) true

@wsor4035 wsor4035 added @ Script API Feature ✨ PRs that add or enhance a feature labels Feb 14, 2024
@Zughy Zughy added Bugfix 🐛 PRs that fix a bug and removed Feature ✨ PRs that add or enhance a feature labels Feb 14, 2024
@y5nw y5nw marked this pull request as ready for review February 14, 2024 18:40
builtin/async/common.lua Outdated Show resolved Hide resolved
builtin/async/common.lua Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Show resolved Hide resolved
games/devtest/mods/unittests/async_env.lua Outdated Show resolved Hide resolved
src/script/common/c_packer.h Outdated Show resolved Hide resolved
builtin/async/common.lua Outdated Show resolved Hide resolved
src/script/common/c_packer.cpp Outdated Show resolved Hide resolved
builtin/common/async.lua Outdated Show resolved Hide resolved
builtin/init.lua Outdated Show resolved Hide resolved
src/script/common/c_packer.cpp Outdated Show resolved Hide resolved
src/script/common/c_packer.cpp Show resolved Hide resolved
src/script/common/c_packer.cpp Show resolved Hide resolved
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Feb 16, 2024
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 16, 2024
@y5nw
Copy link
Contributor Author

y5nw commented Feb 16, 2024

Rebased.

@sfan5 sfan5 removed Rebase needed The PR needs to be rebased by its author. Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Feb 16, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

tested and works
(if the CI fails now that would be embarassing)

@SmallJoker
Copy link
Member

Discussed in the meeting. https://irc.minetest.net/minetest-dev/2024-03-03#i_6156924

Contains more ideas for possible optimizations (not for this PR).

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

The code looks correct. Unit tests pass. I took the liberty to extend the async metatable registration test with a table with a custom, registered metatable.

My comments are nitpicks; do what you want with them.

I rebased because otherwise it wouldn't compile.

builtin/common/metatable.lua Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
games/devtest/mods/unittests/async_env.lua Outdated Show resolved Hide resolved
games/devtest/mods/unittests/async_env.lua Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 4, 2024
@appgurueu appgurueu removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 5, 2024
@y5nw y5nw requested a review from sfan5 March 6, 2024 15:40
@sfan5 sfan5 merged commit fc80f65 into minetest:master Mar 6, 2024
15 checks passed
@y5nw y5nw deleted the async-mt branch March 6, 2024 19:19
y5nw added a commit to y5nw/minetest that referenced this pull request Mar 7, 2024
@sfan5
Copy link
Member

sfan5 commented May 12, 2024

With #14451 it will become relevant to extend this mechanism to the mapgen env too.
I propose a rename to minetest.register_portable_metatable.

@y5nw
Copy link
Contributor Author

y5nw commented May 13, 2024

With #14451 it will become relevant to extend this mechanism to the mapgen env too.
I propose a rename to minetest.register_portable_metatable.

I renamed this to register_metatable in #14360, but register_portable_metatable is IMO better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector metatable doesn't carry over to async environment
6 participants