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

Run LBMs/ABMs as whole mapblocks if registered without nodenames #14723

Open
GreenXenith opened this issue Jun 2, 2024 · 11 comments
Open

Run LBMs/ABMs as whole mapblocks if registered without nodenames #14723

GreenXenith opened this issue Jun 2, 2024 · 11 comments
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API

Comments

@GreenXenith
Copy link
Member

GreenXenith commented Jun 2, 2024

Problem

There is currently no clean way to run a single callback when a mapblock is activated, regardless of nodes.

Solutions

If nodenames are not defined (empty table or nil) the action should be called once on activation with the first pos in the mapblock and a nil node.

For consistency, this behavior should be added to ABMs, running a single action on each interval and ignoring chance.

A separate callback could be used instead but "LoadingBlockModifier" and "ActiveBlockModifier" already exists so why not use them.

@GreenXenith GreenXenith added the Feature request Issues that request the addition or enhancement of a feature label Jun 2, 2024
@sfan5
Copy link
Member

sfan5 commented Jun 2, 2024

related: #12128

@Warr1024
Copy link
Contributor

Warr1024 commented Jun 4, 2024

It might be better to require a specific value that's harder to provide by accident for nodenames for this, like a boolean false, or 0 number or a "*" string or something.

I have cases where I construct ABM definitions programmatically. If I implement them wrong, registering an ABM with an empty or missing list is a possible failure case. In that case, it would save me a lot of debugging hassle if those were just rejected as invalid ABMs (e.g. throw an error) instead of accepting them but triggering significantly different behavior.

@Warr1024
Copy link
Contributor

Warr1024 commented Jun 4, 2024

We could also consider using different names just to make this less error prone. Since we already have Active Block Modifiers and Loading Block Modifiers that operate on nodes, we could have Active Node Modifiers and Loading Node Modifiers that operate on blocks.

@aminothere
Copy link
Contributor

Specifically per-mapblock ABMs would be a wonderful thing I would like to have in minetest!

If I understand the portent of this feature request correctly, this would make it possible to delegate to the lua code to (randomly) select nodes in the mapblock for processing. The lua code could then run node-specific callbacks.

This would allow capping the amount of ABMs run per mapblock, thereby allowing to limit overall the amount of ABM processing. Currently, I see no easy way to do this, other than trying to hack some form of statefulness into ABM handlers.

PS: What is the rationale for ignoring chance? If set to 1 would it not effectively be the same as ignoring?

@sfan5
Copy link
Member

sfan5 commented Jun 4, 2024

I don't see any advantages of shoehorning this into the existing ABM code. It isn't really a good match either.
I think a generalized API that is called on status change for a block and indicates the block's new status (cf. minetest.compare_block_status) makes more sense.

My other thought was: Would mods benefit from a global set minetest.loaded_blocks or minetest.active_blocks indexed by position and maintained by the C++ code? It would also be iterable.

@sfan5 sfan5 removed the @ Mapgen label Jun 4, 2024
@aminothere
Copy link
Contributor

aminothere commented Jun 4, 2024

My other thought was: Would mods benefit from a global set minetest.loaded_blocks or minetest.active_blocks indexed by position and maintained by the C++ code? It would also be iterable.

In a way very much. AFAIK, currently there is no way for the lua code to know what blocks are loaded/active, nor how to iterate such blocks.

But I prefer a model where the lua code provides callbacks that are triggered per block by the c++ code over a model where the c++ code provides a list of loaded/active blocks to be iterated by the lua code.

@GreenXenith
Copy link
Member Author

GreenXenith commented Jun 4, 2024

If I understand the portent of this feature request correctly, this would make it possible to delegate to the lua code to (randomly) select nodes in the mapblock for processing. The lua code could then run node-specific callbacks.

My proposal was that it would run on the whole mapblock at once, and pass the "first" node in the mapblock, representing the mapblock position. It would then be up to the callback to load nodes if it needed to.

PS: What is the rationale for ignoring chance? If set to 1 would it not effectively be the same as ignoring?

Sure, a chance of 1 would be the same. But the point is to run it every time, since an ABM running sporadically on an entire mapblock makes much less sense. The chance exists for regular ABMs since a regular ABM runs on multiple nodes in a mapblock, and chance helps reduce the amount of calls. "Ignored", "considered to be 1", take your pick.

But it looks like better ideas have come up :)

@GreenXenith
Copy link
Member Author

GreenXenith commented Jun 4, 2024

I don't see any advantages of shoehorning this into the existing ABM code. It isn't really a good match either.

Well, I don't really agree that it "isn't a good match" when it is already in the name, but I was also more focused on the LBM side of things anyway (which feels like a better match than ABMs, sure). However...

I think a generalized API that is called on status change for a block and indicates the block's new status (cf. minetest.compare_block_status) makes more sense.
My other thought was: Would mods benefit from a global set minetest.loaded_blocks or minetest.active_blocks indexed by position and maintained by the C++ code? It would also be iterable.

I agree with these. With such methods and registries, LBMs and ABMs could also be altered to rely on this infrastructure, introducing more flexibility.

My question is what data the block tables would have. Many callbacks would appreciate a VoxelManip. As far as I understand, a VM wont actually read any data until asked, so it should be fine to store a few of those and let callbacks call get_data() when desired?

These should also definitely be exposed to the async environment.

@sfan5
Copy link
Member

sfan5 commented Jun 5, 2024

But I prefer a model where the lua code provides callbacks that are triggered per block by the c++ code over a model where the c++ code provides a list of loaded/active blocks to be iterated by the lua code.

It isn't either or the other.
I advocate adding a callback and optionally also a global table mods can easily check.

@sfan5
Copy link
Member

sfan5 commented Jun 5, 2024

With such methods and registries, LBMs and ABMs could also be altered to rely on this infrastructure, introducing more flexibility.

That would introduce additional data copying and (even ignoring that) probably be less efficient than keeping track of this state in C++.

My question is what data the block tables would have.

It would be a set:

minetest.loaded_blocks[minetest.hash_node_position(pos)] == true

As far as I understand, a VM wont actually read any data until asked, so it should be fine to store a few of those and let callbacks call get_data() when desired?

It will. get_data() just moves the copy to the Lua side.
Even if it was free I don't really see the benefit.

@GreenXenith
Copy link
Member Author

It will. get_data() just moves the copy to the Lua side.
Even if it was free I don't really see the benefit.

Ah, well that's no good. The idea would be similar to on_generated callbacks, in order to operate on nodes in the chunk, but it isn't much effort to make the VM when needed anyway, so fair enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API
Projects
None yet
Development

No branches or pull requests

5 participants