Skip to content

Implement non-mapgen VoxelManip functionality#82

Merged
S-S-X merged 16 commits intoS-S-X:masterfrom
TurkeyMcMac:voxelmanip
May 29, 2022
Merged

Implement non-mapgen VoxelManip functionality#82
S-S-X merged 16 commits intoS-S-X:masterfrom
TurkeyMcMac:voxelmanip

Conversation

@TurkeyMcMac
Copy link
Contributor

@TurkeyMcMac TurkeyMcMac commented Apr 17, 2022

Changes:

  • Implements VM functionality that is not exclusive to mapgen VMs.
  • Implements param1 support.
  • Makes copies of nodes returned by world.get_node (for safety/correctness.)

Tests have been added for voxel manipulators.

@TurkeyMcMac TurkeyMcMac marked this pull request as ready for review May 10, 2022 12:41
@S-S-X
Copy link
Owner

S-S-X commented May 10, 2022

Thanks, I'd still like to see if anything can be done about "validation" test set where Technic test set jumped from 2.5 seconds to almost 13.8 seconds.

Did not check actual code yet at all but I'd guess it has something to do with this:

Makes copies of nodes returned by world.get_node (for safety/correctness.)

I'll get back to this later, of course drop comment if you know already what most likely causes this change in performance and also any ideas if it could be made better.

@TurkeyMcMac
Copy link
Contributor Author

Where can I see these validation tests?

@S-S-X
Copy link
Owner

S-S-X commented May 10, 2022

Where can I see these validation tests?

These are not for real validation but I'm just simply running tests implemented for various projects and see if results seems to be somewhat similar, if not then looking if something should be done about it.

For example this: #83 (comment)
And Technic test set I've used here comes from https://github.com/mt-mods/technic/tree/master/technic/spec

@TurkeyMcMac
Copy link
Contributor Author

I think the slowdown is due to Technic force-loading areas using VoxelManips. Before this was basically a no-op.

@S-S-X
Copy link
Owner

S-S-X commented May 10, 2022

I think the slowdown is due to Technic force-loading areas using VoxelManips. Before this was basically a no-op.

Yes that seems to be it, Technic is calling through vm to force loading mapblocks in unloaded areas.

This feels like some black magic and voodoo tricks but read_from_map can be delayed until it is actually needed...
Without touching most of current VM code (I guess there's probably better ways to achieve that but not sure...):

diff --git a/voxelmanip.lua b/voxelmanip.lua
index 2aa8767..822d470 100644
--- a/voxelmanip.lua
+++ b/voxelmanip.lua
@@ -289,19 +289,36 @@ function VoxelManip:set_param2_data(buf)
        return buf
 end
 
+local vm_initializer_mt = {}
+function vm_initializer_mt:__index(key)
+       if VoxelManip[key] then
+               if key == "read_from_map" then
+                       return function(vm, p1, p2)
+                               rawset(vm, "_p1", p1)
+                               rawset(vm, "_p2", p2)
+                       end
+               end
+               setmetatable(self, VoxelManip)
+               if type(self._p1) == "table" and type(self._p2) == "table" then
+                       self:read_from_map(self._p1, self._p2)
+               end
+               return self[key]
+       end
+       return rawget(self, key)
+end
+
 mineunit.export_object(VoxelManip, {
        name = "VoxelManip",
        constructor = function(self, p1, p2)
                local vm = {
                        -- These nodes must not mutate.
                        _nodes = setmetatable({}, nodes_mt),
+                       _p1 = type(p1) == "table" and table.copy(p1) or nil,
+                       _p2 = type(p2) == "table" and table.copy(p2) or nil,
                        _emin = vector.new(1, 1, 1),
                        _emax = vector.new(0, 0, 0),
                }
-               setmetatable(vm, self)
-               if type(p1) == "table" and type(p2) == "table" then
-                       vm:read_from_map(p1, p2)
-               end
+               setmetatable(vm, vm_initializer_mt)
                return vm
        end,
 })

Doing something like this however will give huge performance boost for mod tests where VM is only used to load mapblocks if not loaded already.

I guess other possible fix which would additionally improve generic VM performance would be doing direct world.nodes access with copy-on-write and actually I think that's how world.nodes itself should be to protect against core.get_node(pos).param2 = x (actually I think there was even comment in code about that)

@TurkeyMcMac
Copy link
Contributor Author

Sounds complicated IMO. Since technic only uses VoxelManips when it detects ignore nodes, its tests can be sped up in this way:

diff --git a/technic/spec/hv_network_spec.lua b/technic/spec/hv_network_spec.lua
index b226cbc..37e0045 100644
--- a/technic/spec/hv_network_spec.lua
+++ b/technic/spec/hv_network_spec.lua
@@ -17,6 +17,9 @@ describe("HV machine network", function()
 	-- Tell mods that 1 minute passed already to execute all weird minetest.after hacks.
 	mineunit:execute_globalstep(60)
 
+	-- Speed up tests by avoiding supposed get_node failures.
+	world.set_default_node("air")
+
 	local machines = {
 		"technic:hv_generator",
 		"technic:hv_solar_array",
diff --git a/technic/spec/lv_network_spec.lua b/technic/spec/lv_network_spec.lua
index d75ab06..8cb074d 100644
--- a/technic/spec/lv_network_spec.lua
+++ b/technic/spec/lv_network_spec.lua
@@ -17,6 +17,9 @@ describe("LV machine network", function()
 	-- Tell mods that 1 minute passed already to execute all weird minetest.after hacks.
 	mineunit:execute_globalstep(60)
 
+	-- Speed up tests by avoiding supposed get_node failures.
+	world.set_default_node("air")
+
 	local machines = {
 		"technic:lv_generator",
 		"technic:geothermal",
diff --git a/technic/spec/network_spec.lua b/technic/spec/network_spec.lua
index 027ece0..6c498f9 100644
--- a/technic/spec/network_spec.lua
+++ b/technic/spec/network_spec.lua
@@ -12,6 +12,9 @@ sourcefile("machines/HV/generator")
 
 describe("Power network helper", function()
 
+	-- Speed up tests by avoiding supposed get_node failures.
+	world.set_default_node("air")
+
 	world.layout({
 		{{x=100,y=100,z=100}, "technic:lv_cable"},
 		{{x=101,y=100,z=100}, "technic:lv_cable"},

I believe most mods which use VoxelManips for map loading try getting nodes first, making them fixable in this way too.

Copy link
Owner

@S-S-X S-S-X left a comment

Choose a reason for hiding this comment

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

Yes this is better for Technic mod and it would anyway make more sense to update VoxelManip for world.nodes direct access later instead.
I think this is good enough for now but reminder to update function world.get_node(pos) later to use copy on write instead of always copying.

Merging this one soon.

@S-S-X
Copy link
Owner

S-S-X commented May 29, 2022

Well, mineunit failure because luarocks package manager is broken for GitHub ubuntu containers.
Regular rockspec install results in errors like this:

Error: Failed installing dependency: https://luarocks.org/say-1.3-1.rockspec - Error fetching file: Failed downloading https://github.com/Olivine-Labs/say/archive/v1.3-1.tar.gz - v1.3-1.tar.gz

Best resolution for that probably comes with #72 and dockerhub images.

Should be fine anyway so merging this one.

@S-S-X S-S-X merged commit db70bcd into S-S-X:master May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants