Benchmark from blueprints #63
Conversation
rpdelaney
left a comment
rpdelaney
left a comment
There was a problem hiding this comment.
This PR was sent to me and I mistook it as a request for code review. I'm posting the comments I had because they were already mostly done. I hope they're useful; I love that this work is being done and I'm happy to have a dialogue about it.
That said, this isn't my code so feel free to ignore me if it's not helpful.
src/core/control.lua
Outdated
| local first_ghost = bp_ghost[1] | ||
| local offset = {first_ghost.position.x - first_ent.position.x, first_ghost.position.y - first_ent.position.y} | ||
| for _, bpe in pairs(bp_entity.stack.get_blueprint_entities()) do | ||
| if bpe.name == 'big-mining-drill' and bpe.items ~= nil then |
There was a problem hiding this comment.
How about something like (pseudocode):
if not (bpe.name == 'big-mining-drill' and bpe.items ~= nil) then
continue
end
There was a problem hiding this comment.
The idea is that failing fast is often more readable than another layer of indentation.
src/core/config.rs
Outdated
| pub verbose_metrics: Vec<String>, | ||
| pub strip_prefix: Option<String>, | ||
| pub headless: Option<bool>, | ||
| } |
src/core/control.lua
Outdated
| if req.id.name == 'efficiency-module-3' and req.id.quality == nil then | ||
| s.create_entity{name="stone", amount=10000000, position={bpe.position.x + offset[1], bpe.position.y + offset[2]}} | ||
| elseif req.id.name == 'efficiency-module-3' and req.id.quality == 'uncommon' then | ||
| s.create_entity{name="iron-ore", amount=10000000, position={bpe.position.x + offset[1], bpe.position.y + offset[2]}} | ||
| elseif req.id.name == 'efficiency-module-3' and req.id.quality == 'rare' then | ||
| s.create_entity{name="copper-ore", amount=10000000, position={bpe.position.x + offset[1], bpe.position.y + offset[2]}} | ||
| elseif req.id.name == 'efficiency-module-3' and req.id.quality == 'epic' then | ||
| s.create_entity{name="coal", amount=10000000, position={bpe.position.x + offset[1], bpe.position.y + offset[2]}} | ||
| end |
There was a problem hiding this comment.
You have lots of long elseif chains, such as this one here. This pattern adds a lot of complexity to the code, making it more difficult to maintain and understand. It may be better to use a mapping data structure of some kind for looking up the ore type to place instead. Pseudocode:
resourceMapping = {
[nil] = "stone",
uncommon = "iron-ore",
rare = "copper-ore",
epic = "coal"
}
function tryCreateResource(bpe, req) {
resource = resourceMapping[req.id.quality]
if bpe.name != "big-mining-drill"
or not bpe.items
or not resource
or req.id.name != "efficiency-module-3" then
return
end
s.create_entity{
name = resource,
amount = 10000000,
position = {
bpe.position.x + offset[1],
bpe.position.y + offset[2]
}
}
}
for _, bpe in pairs(bp_entity.stack.get_blueprint_entities()) do
for _, req in pairs(bpe.items) do
tryCreateResource(bpe, req)
end
end
There was a problem hiding this comment.
There is more in this file and elsewhere in the PR that is ripe for such improvements to readability and test-ability. Hope this makes sense :)
|
It seems like your latest changes lost the ability to run for a number of ticks before saving, no? |
Correct, I want to keep benchmarking and blueprinting separate. |
|
Right, but after placing the BP, we need to let it fill up buffers and stuff before you can benchmark it |
… making new functions
Good shout, I'll revert to user input. |
florishafkenscheid
force-pushed
the
benchmark-from-blueprints
branch
from
dc440b4 to
d975d6e
Compare
|
I also wonder if we should be converting all the BPs to saves, then moving those saves somewhere. It would then enable us to check modification times on the bp files vs the converted saves to avoid having to regen stuff if only some bps changed |
|
There's already an --output option, and if the user only changes 3 of 10 bps, they'll just have to work with --pattern or removing bp files from the directory |
…the BlueprintConfig
3 participants
As mentioned, this is not tested at all in belt, though I did do manual testing of the mod stuff. I included a properly set up BP string in the tests dir as well