0ptera / Logistic-Train-Network

Factorio mod adding logistic network for trains.
Other
163 stars 65 forks source link

Add handling of `on_entity_cloned` event #274

Closed robot256 closed 3 years ago

robot256 commented 3 years ago

Error message or bug description
When an area including LTN stops is cloned to a new surface (or presumably within the same surface), the new cloned stops do not get registered in the LTN dispatcher. This is because LTN does not yet handle the on_entity_cloned event. When an area is cloned, no other events are triggered on the newly-created entity.

To Reproduce
Any LuaSurface::clone_area() call that includes an LTN stop. For example, with Space Exploration installed, build a Spaceship with an LTN stop connected to the network on the starting surface. Then launch the spaceship, and re-anchor to the starting surface. The LTN stop on the spaceship will no longer be connected to the network.

Attached is a minimal-reproduction save file: se-ltn-test.zip

  1. Load the save, press CTRL-T to see the list of 8 LTN stops in the network.
  2. Click Spaceship Console to the left, then click the "Launch" button in the GUI that appears.
  3. The spaceship will clone to another surface, and immediately clone back to this one.
  4. The LTN stops will no longer appear in the LTN Manager GUI nor respond when the request constant combinators are turned on.

With the proposed fix in place, the LTN stops will resume functioning when they are cloned back to the original surface.

LTN version
1.16.3

Log file
factorio-current.log

Proposed fix Register on_entity_cloned at init.lua line 258:

script.on_event( {defines.events.script_raised_built, defines.events.script_raised_revive, defines.events.on_entity_cloned}, OnEntityCreated )

Then add handling to the stop-events.lua. I tested the following modified function:

  local entity = event.created_entity or event.entity or event.destination
  if not entity or not entity.valid then return end

  if ltn_stop_entity_names[entity.name] then
    CreateStop(entity)

    --------
    -- Optional, if LTN removes stops that vanish without events or the cloning mod deletes the original with raise_destroy=true
    if event.source and event.source.valid then
      if event.source.surface ~= entity.surface then  
        game.print("LTN Removing Cloned Stop "..event.source.backer_name)
        RemoveStop(event.source.unit_number)  
      end
    end
    --------

  end
end

Note that on_entity_cloned does support event filtering, but I did not use it for this test.

0ptera commented 3 years ago

I don't get the optional part of your code. If the cloning mod deletes the original with raise_destroy=true that should raise script_raised_destroy.

robot256 commented 3 years ago

The cloning operation itself does not destroy the source entity. I had an issue originally with the source of the clone being subsequently deleted with raise_destroy not specified (defaults to false), which is why I added it in my handling of on_entity_cloned. But I believe it was since added, at least to Space Exploration, and raise_destroy being intentionally omitted for performance reasons, per the comment in SE mentioning the impact of adding raise_destroy for mod compatibility. It does raise_destroy for everything now. I tested this change in LTN both with and without the optional part and it didn't leave bogus stations in the LTN Manager list either way, so raise_destroy is working.

The performance problem is undoubtedly because script_raised_destroy doesn't support filters, so every mod has to go through everything that gets deleted.

robot256 commented 3 years ago

The update works perfectly. Thank you!