Skip to content

Implement non-mapgen VoxelManip functionality#82

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

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

Conversation

@TurkeyMcMac

@TurkeyMcMac TurkeyMcMac commented Apr 17, 2022

Copy link
Copy Markdown
Contributor

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

S-S-X commented May 10, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
Contributor Author

Where can I see these validation tests?

@S-S-X

S-S-X commented May 10, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

S-S-X commented May 10, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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.

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

Copy link
Copy Markdown
Owner

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

S-S-X commented May 29, 2022

Copy link
Copy Markdown
Owner

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