Caravan Optimizations#567
Conversation
minor simplification
Got rid of CaravanFuncs
Got rid of CaravanFuncs
Got rid of CaravanFuncs
CaravanFuncs
Got rid of CaravanFuncs
|
Did you end up solving the difficulties with caravans not waking up after a long idle? |
protocol-1903
left a comment
protocol-1903
left a comment
There was a problem hiding this comment.
Please add as many annotations as possible, I'm trying to get caravans understandible to new readers without having a PHD in spaghetti reading
I believe I solved the local issue, but caravans stalling is an entire class of problems that needs a generic "kick after x seconds of inactivity" sort of safety check. It isn't what you should do as these situations should never happen, but problems like this are difficult to catch and debug so it is a practical concession. |
Could you give me more words about what you are looking for here? Maybe point to a good file somewhere? |
Any files you modified should be annotated, specifically the parts modified by you |
|
Apologies for the delay on the review, life has been busy. It's still on my list. |
|
Can you explain Is there a reason why it can't be part of the |
There was a problem hiding this comment.
The implementation here feels haphazard. I don't want to throw out your work, but I'm wondering the goals (and maintainability) would be better served by using the lessons learned here to plan a clean re-implementation.
Do you know how much of the gains are simply from toggling .active on the idle caravans? The dual queue feels unwieldy, having to count every iteration to decide when to process the slow queue. The same for counting every caravan action for reasons I haven't yet figured out. I feel like we're doing a lot of "work" to avoid processing the queue - which may not take that long.
|
|
||
| -- wake up! if it happens to be in deep sleep | ||
| if storage.caravans[unit_number].entity ~= nil then | ||
| storage.caravans[unit_number].entity.active = true |
There was a problem hiding this comment.
From the docs: "Writing to this is deprecated and affects only the disabled_by_script state."
disabled_by_script should be written instead
| storage.caravan_fast_queue = nil | ||
| storage.caravan_slow_queue = nil |
There was a problem hiding this comment.
nilling the table adds needless complexity where we constantly have to check if it exists. We should define the table in on_init/on_configuration_changed and simply leave it empty when we aren't using it
| local caravan_data = storage.caravans[tags.unit_number] | ||
| local schedule = caravan_data.schedule[tags.schedule_id] | ||
|
|
||
| CaravanImpl.wake_up( tags.unit_number ) |
There was a problem hiding this comment.
There is a trailing space on this line, along with many others across the PR. It's worth using a linter/formatter with the appropriate settings to prevent this.
I will fix these with a formatting pass when the PR is ready to merge, so don't worry about it for this PR.
| if entity.operable then storage.make_operable_next_tick[#storage.make_operable_next_tick + 1] = entity end | ||
| entity.operable = false -- Prevents the player from opening the gui of the clicked entity | ||
| if entity.name == "outpost" or entity.name == "outpost-fluid" or entity.name == "outpost-aerial" or entity.name == "outpost-aerial-fluid" then | ||
| if entity.name == "outpost" or entity.name == "outpost-fluid" or entity.name == "outpost-aerial" then |
There was a problem hiding this comment.
merge removed the aerial fluid caravan check here
| if not storage.caravan_queue then | ||
| local queue = {} | ||
| local guis_to_update = {} | ||
| local counter = 0 |
|
|
||
| -- Under some circumstances caravans can become stuck in wander mode | ||
| -- why? don't know. But the solution is to tell them to get back to work. | ||
| if not (caravan_data.entity and caravan_data.entity.valid and caravan_data.entity.commandable) then |
There was a problem hiding this comment.
In what cases can the commandable be nil?
|
|
||
| local cmd = caravan_data.entity.commandable.command | ||
|
|
||
| if not (cmd and cmd.type == defines.command.wander) then |
There was a problem hiding this comment.
if not cmd or cmd.type ~= defines.command.wander would be more legible
| if not (schedule and schedule.entity and schedule.entity.valid and schedule.entity.surface == caravan_data.entity.surface) then | ||
| return false | ||
| end |
There was a problem hiding this comment.
How does this work for schedules with a position instead of an entity?
| local action_id = caravan_data.action_id | ||
| if action_id < 0 then action_id = 1 end |
| @@ -324,4 +324,5 @@ function P.restore_gui_location(element, fallback_location) | |||
| end | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
Was this single newline intentionally added?
3 participants
Changes:
Caravan speed optimizations. Idle caravans are now processed less often, leading to substantial performance improvements in large bases.
Minor Caravan schedule UI improvements.