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

Add bulk ABMs #12128

Closed
wants to merge 8 commits into from
Closed

Add bulk ABMs #12128

wants to merge 8 commits into from

Conversation

TurkeyMcMac
Copy link
Contributor

@TurkeyMcMac TurkeyMcMac commented Mar 12, 2022

Would fix #11400 if merged. Usecases of this PR are mentioned there.

The PR adds "bulk ABMs" which execute up to once per ABM run with a list of node positions. A list of nodes is not provided, as this list may become out of date. By the time the Lua callback is called, the nodes may have changed, so the callback should do its own checking. To make an ABM run in bulk, put the action in the field bulk_action instead of action. The action will then be given a list of positions when it runs.

The PR adds a new abstract subclass of ActiveBlockModifier called BulkActiveBlockModifier. Another class LuaBulkABM was also added that implements BulkActiveBlockModifier. The functionality shared between LuaABM and LuaBulkABM has been put into a common parent class AbstractLuaABM.

To do

This PR is ready for review.

  • A way to limit the time taken by bulk ABMs as with other ABMs.
  • Documentation
  • minetest.features entry
  • Tests

How to test

I have added a moving test node testnodes:bulk_abm.

You can also test with this mod: vm_liquid.zip

It's sort of like a simpler version of dynamic_liquid. It makes water sources flow downward and sideways into the available space. Because it uses a bulk ABM, it can make use of voxel manipulators. On my computer, the ABM's bulk_action takes about 20 milliseconds to run with about 10000 nodes. This does not include the time taken by the C++ code scanning for nodes and stuff.

To test the ABM time limit, place a huge area of water sources with World Edit, then let these flow until the ABM time starts hitting the limit. The time shouldn't exceed the limit by too much.

Also make sure regular ABMs still work.

@Desour
Copy link
Member

Desour commented Mar 12, 2022

"bulk ABMs" which execute up to once per ABM run with a list of node positions.

Suggestion: Instead give an object that you can iterate over, so that a pull-style api will be easier to implement, if we want to at some point. (The object can be a table with a metatable created in builtin from a list of positons.)

Also, would it make sense to remove the non-bulk abms in the C++ code, and implement them in builtin using the new bulk abm api?

@TurkeyMcMac
Copy link
Contributor Author

Suggestion: Instead give an object that you can iterate over, so that a pull-style api will be easier to implement, if we want to at some point. (The object can be a table with a metatable created in builtin from a list of positons.)

I'd be OK with this as long as it's still possible to get the positions all at once. As you can see, my example mod relies on random access into the position list. I'll wait on implementing this feature until a core dev voices support for it.

Also, would it make sense to remove the non-bulk abms in the C++ code, and implement them in builtin using the new bulk abm api?

Currently bulk ABMs do not have a throttling mechanism like regular ABMs do. Nor do they have access to active object counts. Unless/until bulk ABMs get these features, the two ABM types should be kept separate in C++.

@TurkeyMcMac
Copy link
Contributor Author

I have added a more substantial example that uses voxel manipulators.

@sfan5 sfan5 added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Mar 19, 2022
@sfan5
Copy link
Member

sfan5 commented Mar 19, 2022

Because it uses a bulk ABM, it can make use of voxel manipulators.

The "so it can use vmanips" part of the example isn't very convincing. The example mod uses one VoxelManipulation per node it potentially touches, which is not particularly efficient. bulk_set_node could very well be faster.

Now bulk ABMs do definitely make using VoxelManips possible, it just requires extra work on the part of mods. You'd group node positions together to form one (or multiple) cuboids. Then there'll be a cutoff where using a vmanip is generally worth it performance-wise.

@TurkeyMcMac
Copy link
Contributor Author

TurkeyMcMac commented Mar 19, 2022

The example mod uses one VoxelManipulation per node it potentially touches, which is not particularly efficient.

No, it uses one VM per block it touches.

When a lot of liquid is moving at once, there are usually many moving nodes per block.

I think loading all the data at once also avoids LuaJIT trace aborts when doing the actual processing.

@TurkeyMcMac
Copy link
Contributor Author

That said, I haven't tested the speed of the ABM when voxel manipulators are not used.

@sfan5
Copy link
Member

sfan5 commented Mar 20, 2022

No, it uses one VM per block it touches.

Oops, missed that. It does pretty much what I suggested.

@sfan5 sfan5 added WIP The PR is still being worked on by its author and not ready yet. Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Apr 1, 2022
@sfan5
Copy link
Member

sfan5 commented Jun 27, 2022

Kind of obvious, but: LBMs would benefit from having the same feature (and you won't even have to worry about time budget for them)

@TurkeyMcMac
Copy link
Contributor Author

Are there LBMs that act on large numbers of nodes or that would otherwise benefit from a bulk option?

@sfan5
Copy link
Member

sfan5 commented Jul 3, 2022

Well here's something close to my usecase:

minetest.register_lbm({
	name = "default:do_thing",
	nodenames = {"default:dirt"},
	run_at_every_load = true,
	action = function(pos, node)
		local p1 = vector.offset(pos, 0, 1, 0)
		local above = minetest.get_node(p1)
		if above.name == "air" and minetest.get_node_light(p1) >= 13 then
			node.name = "default:dirt_with_grass"
			minetest.swap_node(pos, node)
		end
	end,
})

I think the optimization potential is obvious.

@Zughy
Copy link
Member

Zughy commented Aug 13, 2022

@TurkeyMcMac any update?

@TurkeyMcMac
Copy link
Contributor Author

I should work on this more. I suppose I'll add bulk LBMs. The main thing keeping this WIP is the question of how to limit the runtime of bulk ABMs.

@sfan5
Copy link
Member

sfan5 commented Aug 13, 2022

Maybe we could start by adding bulk LBMs which don't have any runtime cap concerns? (I might work on those otherwise)

@TurkeyMcMac
Copy link
Contributor Author

@sfan5 Yes, bulk LBMs could be a separate PR from this one.

@TurkeyMcMac
Copy link
Contributor Author

To be clear, I don't have work to do on this PR at the moment. I might open a bulk LBM PR at some point, but someone else can implement that if they want.

@TurkeyMcMac
Copy link
Contributor Author

Here's an idea for limiting the duration of bulk ABMs: record the average execution time per node, then add that this amount to the total ABM execution time for every node you record as later input to a bulk ABM.

@TurkeyMcMac TurkeyMcMac force-pushed the bulk-abm branch 2 times, most recently from 18854a0 to 7cc4249 Compare October 17, 2022 19:34
@TurkeyMcMac TurkeyMcMac marked this pull request as ready for review October 17, 2022 19:35
@TurkeyMcMac TurkeyMcMac removed WIP The PR is still being worked on by its author and not ready yet. Help needed labels Oct 17, 2022
@rubenwardy rubenwardy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Feb 6, 2023
@Zughy Zughy closed this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adoption needed The pull request needs someone to adopt it. Adoption welcomed! Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Server / Client / Env.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stateful ABM runs
5 participants