-
Notifications
You must be signed in to change notification settings - Fork 10
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
Technic compatibility with fully contained network move capability #79
base: master
Are you sure you want to change the base?
Conversation
for x=0,size.x do | ||
for y=0,size.y do | ||
for z=0,size.z do | ||
local local_pos = {x=x,y=y,z=z} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem here is when large areas are jumped, this most probably is not even near most efficient method to check nodes in area.
Any suggestions for better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to find_nodes_in_area and include cable groups and machine group, if that is significantly faster way to get nodes then loop here could be a lot shorter especially for large areas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm, this does not sound really efficient, would that be another use-case in that #47 could help with. It would need to be defined on every cable-definition (and possibly on the switching station):
minetest.register_node("my:node", {
on_movenode = function(from_pos, to_pos)
end
})
This way the nested for-loops could be replaced with some logic within the jumpdrive mod (one nested for-loop is better than 2 of them, right?).
Let me know if something like that would help and if the parameters are enough (from- and to-position) i think i have some time to implement this and rewrite the jump-compatibility functions (or even move it to the external mods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thing is with R15 it is still well over 50% faster than rebuilding network. I think mostly because it is very rare to read any nodes from world (only around edges of box when there is also active network at location) but other than that it is pure math.
But also can clearly see how performance degrades when area gets bigger, that is because of looping over all locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the nested for-loops could be replaced with some logic within the jumpdrive mod (one nested for-loop is better than 2 of them, right?).
Node move cannot be used alone, did you notice that this loop also checks taget and source around jumped area for possible network connections at target and cable cutting at source.
It could be used if there's information about location in jumped area so that connections across edge can be checked.
But it would work yes with few additions and possibly better than blindly looping through whole area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would provide a way to react on single nodes and to process/finalize the whole after a jump:
local jumped_cables = {}
minetest.register_node("technic:cable_xy", {
on_movenode = function(from, to, additional_info)
-- TODO: collect cable-positions here
table.insert(jumped_cables, { from=from, to=to, edge=additional_info.edge })
end
})
-- callback for jump finalization stage (for example savefile writes)
jumpdrive.register_after_jump(function(from_area, to_area)
-- from_area = { pos1, pos2 }
-- to_area = { pos1, pos2 }
-- TODO: finalize (somehow)
for _, entry in ipairs(jumped_cables) do
-- magic
end
-- reset list for next jump
jumped_cables = {}
end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so additional_info.edge
would contain 1-3 edge directions / vector representing directions, that would work yes.
Most of logic from this PR could be moved to finalization and if edge
provides vector for 90 degree directions relative to edge wall then that could also be skipped in node move implementation for networks.
I think that could be useful for some other things too that can depend on nearby stuff outside of source or target area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, tried with following. Removed nested loops and used 254 nodes instead of 29791 nodes.
those nested loops were about 8 times faster...
local nodes = minetest.find_nodes_in_area(source_pos1, source_pos2, {
"group:technic_machine",
"group:technic_lv_cable",
"group:technic_mv_cable",
"group:technic_hv_cable",
})
-- search results
local jump_networks = {} -- jumped fully contained networks
local edge_networks = {} -- networks crossing jumped area edge
-- search for networks in area
local size = vector.apply(vector.subtract(source_pos1, source_pos2),math.abs)
--for x=0,size.x do
--for y=0,size.y do
--for z=0,size.z do
for _, pos in ipairs(nodes) do
....
actually after testing more hoping to get caches warmed those nested loops seem to be over 10 times faster on average
tested this with R15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional_info.edge would contain 1-3 edge directions / vector representing directions, that would work yes.
Hmm, it does not (yet) only a boolean value if it is on the edge 😋
But i guess returning a vector for the edge-position should be fairly easy, something along the lines of this:
-- only an edge to the lower/y direction
edge = { x=0, y=-1, z=0 }
-- on an upper corner
egde = { x=1, y=1, z=-1 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could put up some simple test for this and if it still seems that nested loops using just pure math is better then maybe good option would be to implement actual network moving in technic mod instead and just call some technic.move_network(source1, source2, target1)
from here.
But seems that needs some testing, and possibility to register move callback for nodes will be useful no matter how network data moving will be actually implemented.
Currently just for single engine ships. If networks are fully contained this should be a lot faster and cheaper compared to full network rebuilds.
Also if someone who knows math can make it better then.. well, it probably will be better.
@OgelGames @BuckarooBanzay do you see problems here (other than nested loops, those never look pretty)
This supersedes #78 that I investigated a bit and realized that it wont work.
Testing with R7 contained network moving network with this takes about 20% of rebuilding network.
This is pretty much required to make new networks jumpable because those will not lose cache that easily.