Group assignment lost when splitting wagon from locomotive

[robot256]

Describe the Bug

Train group assignment is lost when one train is split into two, and one of the new trains has no locomotives. This is because the on_train_created event is called twice with the same old_train_1 parameter, but groups.migrate_trains purges the group data after handling only the first event, if the first new train happened to have only wagons. When the second event occurs for the new train that has locomotives, there is no train data referring to the old train ID.

I found this bug because I received a report about Multiple Unit Train Control interfering with Train Groups, but I was able to reproduce it without any involvement of MUTC.

To Reproduce

Steps to reproduce the behavior:

1. Place a train consisting of one locomotive and one wagon.
2. Assign a group to the locomotive.
3. Enter the wagon as a passenger.
4. Press 'V' to uncouple the wagon from the locomotive. (Riding in the wagon forces the wagon to be referenced in the first firing of on_train_created.)
5. Observe the locomotive no longer has a group assigned.
6. In the log file, observe that the data for the original train was removed before attempting to migrate the new train with the locomotive:

   1056.239 Script @TrainGroups/control.lua:10: ADD TRAIN: [612] 1056.239 Script @TrainGroups/control.lua:10: CHANGE TRAIN GROUP: [612] | [] -> [mygroup] 1060.222 Script @TrainGroups/control.lua:10: REMOVE TRAIN: [612] 1060.222 Script @TrainGroups/control.lua:10: CHANGE TRAIN GROUP: [612] | [mygroup] -> [] 1060.222 Script @TrainGroups/control.lua:10: MIGRATE TRAIN: [614] <- [612] [nil]

Save file

traingroup_bug.zip

Log file

I added an additional logging step to indicate when a migrated train was chosen for removal because it had no locomotives (the "DELETE" lines). factorio-current.log

[robot256]

Given how many times on_train_created can be called for one train in one tick, I would suggest keeping track of all the changes in train IDs without deleting any data within the on_train_created event. Then set a timer for the next tick to clean up any references to trains that don't exist anymore, maybe with a temporary list of trains that were touched so you don't have to check every single one.

[raiguard]

This is actually hilarious. I originally did keep track of trains to be deleted on the next tick exactly as you suggested, but removed it because I couldn't remember why it was there (there was a year and a half where I stopped working on this mod). I guess this was why!

[robot256]

Haha! Now we have a paper trail so you won't forget 😎

[raiguard]

I really want to avoid adding an on_tick handler if possible, so I found a much more elegant solution. Turns out I have some strange logic where if the new train had no locomotives, it deleted both old trains' group data. That made zero sense, so I removed that and changed it to only add the new train if it has locomotives. That fixed the problem!