Electric network overhaul! - #69
Conversation
|
I download this fork and got reducing in electric network from 8.5 to 4.9 on solar only base, I have 120 buildings |
If this PR gets accepted I'll make another PR with optimizations to belt connections. That should reduce UPS usage even further. Going from 8.5 to 4.9 is still a great improvement. |
|
Tried it, works wonderfully. Cleaner electric network info interface and improved performance. |
mrudat
left a comment
mrudat
left a comment
There was a problem hiding this comment.
Just nitpicking, but there are a few comments where ' has been replaced by ".
| if neighbour ~= pole then old[#old+1] = neighbour end | ||
| end | ||
| dc.disconnect_neighbour() | ||
| for _, neighbour in ipairs(old) do |
There was a problem hiding this comment.
Using ipairs is creating a lot of weird code in this PR, such as the old[#old + 1],
It would be cleaner to use pairs, and to index by the poles unit number
| factory.inside_energy_receiver.destroy() | ||
| local function delete_middleman(i) | ||
| local pole = global.middleman_power_poles[i] | ||
| if pole == 0 then return end |
There was a problem hiding this comment.
Why is the pole entry not set to nil? setting it to 0 is strange
There was a problem hiding this comment.
This is a necessary step for this line
local middleman = surface.create_entity{name = "factory-power-connection", position = {2*(n%32), 2*math.floor(n/32)}, force = "neutral"}
The value "n" is calculated by searching for the first 0 aka gap in the pole grid.
If it was set to nil then I would not be able to find this gap.
There are other ways of solving this problem but I believe this is the cleanest
| factory.outside_energy_sender = new_oes | ||
| local function cleanup_middlemen() | ||
| for i, pole in ipairs(global.middleman_power_poles) do | ||
| if pole ~= 0 and #pole.neighbours.copper<2 then delete_middleman(i) end |
There was a problem hiding this comment.
Another strange place, checking that pole ~= 0.
If you use pairs, and clear the pole to nil, then you can just use a normal loop
Also you should be checking if the pole entity is valid
| for i, pole in ipairs(global.middleman_power_poles) do | ||
| if pole == 0 then n = i break end | ||
| end | ||
| n = n or #global.middleman_power_poles + 1 |
There was a problem hiding this comment.
This is also a continuation of the strange code pattern.
This would be a lot easier, cleaner, and readable, if you use the unit_number as the index, and loop using pairs
|
|
||
| -- find the nearest connected power pole | ||
| local D = game.max_electric_pole_supply_area_distance + factory.layout.outside_size / 2 | ||
| local canidates = {} |
| local canidates = {} | ||
| for _, entity in ipairs(surface.find_entities_filtered{type="electric-pole", area={{x-D, y-D}, {x+D,y+D}}}) do | ||
| if entity.electric_network_id == electric_network and entity ~= pole then | ||
| canidates[#canidates+1] = entity |
There was a problem hiding this comment.
You can use the index from the loop (_ in this case, or rename it to k),
That way it won't have to recalculate the size every addition
Also then you must loop with pairs, since it will be sparse
But it is recommended to always loop with pairs in factorio, as performance is the same and it handles all the cases better
|
How do I download this to use it in my game? I am clueless how to use github as I've only used it for bug reporting for another game. Really need the performance boost... |
|
maybe you can merge first and then make the code cleaner later, so we can use the optimized mod in game as soon as possible @Kolnan. How about that? |
|
any updates? |
|
Any updates on this? |
I don't think mcmagmafry intends to merge this fork. If he wants to merge then I don't mind doing an update. |
|
If he so chooses to merge this, I would very much appreciate the performance boost, if it's as good as you guys are saying. My Game update for Electric network alone is almost at 12, literally everything else combined doesn't generate as much of a performance hit as this mod does. The script update for Factorissimo2 is only at ~2 so I'm not having any issues there, I don't think. |
|
Any estimate on when this will be merged? I really love the mod, but keep running into performance issues and want to try this out on a new save |
MagmaMcFry has been away from the Factorio community. Unfortunately I'm not sure if he will ever merge this. I should mention that this PR does not require a new save because it has migrations. |
Late reply, sorry. That's very.. unfortunate. Is there some way you're able to merge it or can only MagmaMcFry merge it? |
|
I can't do it
…On Sun, May 22, 2022 at 3:09 PM Ami ***@***.***> wrote:
Any estimate on when this will be merged? I really love the mod, but keep
running into performance issues and want to try this out on a new save
MagmaMcFry has been away from the Factorio community. Unfortunately I'm
not sure if he will ever merge this. I should mention that this PR does not
require a new save because it has migrations.
Late reply, sorry. That's very.. unfortunate. Is there some way you're
able to merge it or can only MagmaMcFry merge it?
—
Reply to this email directly, view it on GitHub
<#69 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHGNM4PLZPYIDTGG2L2BCOLVLKH6VANCNFSM5GMVYTQA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I see. That sucks. I hope MagmaMcFry maybe comes back and merges it. Alternatively, how do I use this myself for my own game? |
|
I managed to figure out how to download this, and I must say that it runs muuuuch better now. Many thanks. |
|
This fork has been released on the mod portal |
8 participants
This PR changes the factory electric network system from using electric energy interfaces to using cross-surface electric poles.
From my testing, this reduced the UPS usage of the mod by over 70%
(Vanilla electric network calcuations ~3.3ms -> ~0.3ms) (Factorissimo script update ~2.2ms -> ~1.1ms)
These values were recorded on my factory recursion base with over 200 factory buildings.
There is also the nice side effect that acculumulators function properly in factory buildings.
This PR also overhauls the factory lamp upgrade. There are no longer any actual lamp entities. The surface is instead set to permanent daytime whenever you research the upgrade. (Solar panels still do not work in factory buildings)
The rationale for this change is the vanilla bug where having a surface in nighttime corrupts all solar panes on connected surfaces.
Removing lamps also heavily decreases the entity count
Migrations are also included
July 8 edit: This fork has been released on the mod portal
https://mods.factorio.com/mod/factorissimo-2-notnotmelon