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

Allow passing arbitrary data to mapgen Lua env (also at runtime) #14451

Open
aerkiaga opened this issue Mar 10, 2024 · 5 comments · May be fixed by #14659
Open

Allow passing arbitrary data to mapgen Lua env (also at runtime) #14451

aerkiaga opened this issue Mar 10, 2024 · 5 comments · May be fixed by #14659
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature request Issues that request the addition or enhancement of a feature @ Script API

Comments

@aerkiaga
Copy link
Contributor

Problem

Mods with custom mapgens may want to generate terrain differently in response to game events. This use case is not currently handled by the asynchronous mapgen API, as there is no way to pass information into the isolated mapgen environment.

Solutions

The idea is that the main environment should be able to write data at any time into some sort of shared storage, which the mapgen thread would be able to retrieve whenever a new area is generated. Ideally, this would only be writeable by the main environment, so locking could be performed optimally in the form of a read-write lock.

Alternatives

As @sfan5 suggested, a workaround that currently works is passing temporary files across environments. I have tested this to good results.

Additional context

#14450

@aerkiaga aerkiaga added the Feature request Issues that request the addition or enhancement of a feature label Mar 10, 2024
@sfan5 sfan5 changed the title Allow passing arbitrary data to mapgen env Allow passing arbitrary data to mapgen Lua env (at runtime) Mar 10, 2024
@sfan5 sfan5 added @ Script API Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR labels Mar 10, 2024
@sfan5 sfan5 changed the title Allow passing arbitrary data to mapgen Lua env (at runtime) Allow passing arbitrary data to mapgen Lua env (also at runtime) Mar 10, 2024
@Zughy Zughy added Concept approved Approved by a core dev: PRs welcomed! and removed Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR labels Mar 11, 2024
@sfan5
Copy link
Member

sfan5 commented May 12, 2024

I initially wanted to design something one-way that is specific to mapgen threads but I guess there's no way around just adding a general IPC mechanism.
Modders will absolutely do stupid stuff with it but it's also very powerful 😃

Here's my proposal:

IPC

The engine provides a generalized mechanism to enable sharing data between the
different Lua environments (main, mapgen and async).

  • minetest.ipc: table
    • Writing an arbitrary Lua values to this table will instantly propagate
      it to the same table in all other Lua environments.
    • The Lua values cannot be or contain userdata.
    • Keys must be strings and should use the "modname:thing" notation to avoid conflicts.

Note that the table does not actually contain any data. Indexing it will trigger
a call to C++ and perform an operation comparable to (de)serialization.
Therefore the following idiom will NOT work:

minetest.ipc["test:foo"] = {} -- initialize

minetest.ipc["test:foo"].subkey = "value"

Correct usage needs to look as follows:

minetest.ipc["test:foo"] = {} -- initialize

local tmp = minetest.ipc["test:foo"]
tmp.subkey = "value"
minetest.ipc["test:foo"] = tmp

As for the implementation the Server class would gain an std::unordered_map<std::string, std::unique_ptr<PackedValue>> to contain the data. Also this will be relevant.
Given the description it should be obvious that the Lua side will use __index and __newindex meta-methods.

Edit: Locking primitives intentionally don't exist here because most of MT's threads are not supposed to be blocked at random points. I can see myself adding CAS however if it turns out to be useful.

@grorp
Copy link
Member

grorp commented May 12, 2024

Therefore the following idiom will NOT work: [...]
Correct usage needs to look as follows: [...]

To prevent API users from doing the wrong thing in the first place, wouldn't an API like minetest.ipc_set(key, value) / minetest.ipc_get(key) -> value make more sense?

What use cases do you have in mind other than the one described in this issue?

@sfan5
Copy link
Member

sfan5 commented May 12, 2024

To prevent API users from doing the wrong thing in the first place, wouldn't an API like minetest.ipc_set(key, value) / minetest.ipc_get(key) -> value make more sense?

I considered that but thought that the usability/readability improvement from a metatable is better.
No strong feelings however.

What use cases do you have in mind other than the one described in this issue?

It works very well to transfer global data into async or mapgen env (like configuration).
Nothing more, but people will inevitably find some fun thing to do.

@appgurueu
Copy link
Contributor

I think the metatable-based API, while providing nice syntactic sugar, is not worth the footguns it brings. Modders can usually expect tables to behave reasonably, with pretty much the only exception being __index used to implement method calls (and even that often confuses novices, and sometimes even experienced modders).

This "table" however is special: Some values can't go in it. Subtables can't go in it. pairs can't iterate over it, and there is no way to fix this (without patching pairs to respect a __pairs metamethod, which PUC Lua 5.1 doesn't support). rawget / rawset may lead to surprises. Performance will be very different from a normal table.

I don't think it's good to create the illusion that this is anything like a table. Too many footguns in that direction; it's better to be explicit here. If experienced modders think they want the syntactic sugar, it's easy enough to implement in < 10 lines of Lua.

As a "compromise" solution, having something like a global ipc singleton / namespace might also be convenient. The calls would then look like minetest.ipc.set(k, v) / minetest.ipc.get(k); further methods - say, minetest.ipc.pairs() - would then be easy to add. Modders could localize the ipc namespace or even single functions for convenience.

@sfan5
Copy link
Member

sfan5 commented May 12, 2024

__pairs metamethod, which PUC Lua 5.1 doesn't support

FWIW we no longer support PUC Lua so this would actually not be a problem.

The calls would then look like minetest.ipc.set(k, v) / minetest.ipc.get(k)

Well that's not so different from ipc_set and ipc_get. I'll go with that.

minetest.ipc.pairs()

I planned to not support iterating the global IPC data table. Mods should know what they set.
This has the nice side effects that mods being able to use a randomly generated key if they really really want to avoid other code touching their data.

@sfan5 sfan5 linked a pull request May 14, 2024 that will close this issue
2 tasks
@sfan5 sfan5 linked a pull request May 24, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature request Issues that request the addition or enhancement of a feature @ Script API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants