This is an interaction with the Miniloader (Redux) mod "https://mods.factorio.com/mod/miniloader-redux", which has modded buildings. In the attached save, at [gps=1049.6,1659.7] I have a red miniloader in load mode marked for downgrade to yellow miniloader. If I shoot the blueprint shotgun at that miniloader to try to downgrade it, the miniloader turns into a glitched building. The glitched building looks like a red miniloader, but I can't mark it for destruction or upgrade, I can't mine it by hand, I can't open its configuration popup, I can build a small power pole or iron chest over it, and it no longer loads items into the big chest. I don't see a way to remove this glitched building without cheats.

Save: https://drive.google.com/file/d/1D0zN0n3ODJIq1T0Dss-mmLi39dgOHwxt/view?usp=sharing

Versions: Factorio 2.0.67 (build 84451 expansion, linux64); Blueprint Shotgun 0.2.20; Miniloader (Redux) 0.13.0

Bug crossposted to "https://mods.factorio.com/mod/miniloader-redux/discussion/6a0ddc50193b25114f0ecb97"

This is the same problem as documented here: https://mods.factorio.com/mod/blueprint-sandboxes/discussion/69f987c16a415897c8875d6c

If the code does not use entity.apply_upgrade() but tries to "emulate" upgrading by using create_entity, the game will not fire the necessary events that the miniloader needs to be completely constructed.

This is a bug in blueprint shotgun (and in blueprint sandboxes for that matter).

As @_CodeGreen has disabled pull requests and bug reporting, feel free to pick up https://github.com/hgschmie/factorio-blueprint-shotgun/commit/6216ac8d2f628b28cda9db7a7a50401deeaacddc which will fix this problem.

There is nothing that can be done from the miniloader side. Then current way to upgrade entities does not fire the necessary events for the miniloader to react to.

Did you test that code at all? It doesn't even do the same thing as before, and in fact voids items when upgrading entities instead of dropping them on the ground like it's supposed to. This condition doesn't even do what it's supposed to, entity.to_be_upgraded is a function, not a boolean read, and even if it was being called it would be checking if the upgrade proxy entity is marked for upgrade, not its target, so that part would never be true:
if not (entity.valid and entity.to_be_upgraded) then return end

create_entity with fast_replace = true is a normal way for mods to replace entities, especially in cases where other parameters of the API call are used such as player, or in my case spill = true. I'm not "emulating" an upgrade, and I do not want to script apply an upgrade, because it's not accomplishing the intended result and causes more issues that I would have to deal with. While create_entity can be used for the same thing as apply_upgrade, they are not the same thing, and create_entity has much more capability than the latter.

You say there's nothing that can be done from the miniloader side, I disagree. I took a look at the code, this is so easily fixed by using on_object_destroyed for non-ghost miniloaders as well. That would solve both the issue with this mod, the one with blueprint sandboxes, and any other mod doing the same.
Stop blaming other mods for using the API as it was intended.

I tested that it correctly creates miniloaders. I did not really deal with the item management or the other parts. There are probably typos in there like the one you found with to_be_upgraded.

I ran a patched version of the mod, selected some miniloaders for upgrade and downgrade and could shoot at them to upgrade and downgrade them correctly.

The create_entity call does not generate the script_raised_create event for the fast-replaced miniloader, which is why the results are incomplete. I remembered this wrong. The create_entity call does not create a 'script_raised_destroy' event for the old miniloader.

https://lua-api.factorio.com/latest/classes/LuaEntity.html#apply_upgrade exists to upgrade entities marked for upgrading. And you do not need to select spilling because e.g. underground belts simply retain their content.

The problem is not "on_object destroyed". The problem is the missing "script_raised_create" event for the upgraded loader which is why you only get the initial inserter (and not the additional "hidden" entities).

[ I remembered that wrong; when I wrote it I was not at my computer at home and could not look at the actual events. The problem is that the code simply tries to "replace in place" and that leads to collisions with the hidden entities. The miniloader sees the creation events but misses the deletion events for the destroyed "old" miniloader]

As I use neither blueprint shotgun nor blueprint sandboxes, what I can do is pointing out the interaction problems. If you think that there are things that I can do, I will happily apply them to the miniloaders (I take PRs) but other than that, I encourage you to review your code.

This is the event that is currently fired when using blueprint shotgun to upgrade a fast miniloader to an express miniloader:

Received Creation Event: {
created_entity = "[LuaEntity: hps__ml-express-miniloader at [gps=19.5,-1.5]]",
define_name = "script_raised_built",
entity = "SERPENT PLACEHOLDER",
name = 92,
options = {},
tick = 48550
}

It is a single creation event for a new miniloader (express miniloader). The fast miniloader is still there and the creation fails.

When changing the code to use apply_upgrade, it does this:

Received Deletion Event: {
define_name = "script_raised_destroy",
entity = "[LuaEntity: hps__ml-fast-miniloader at [gps=18.5,-1.5]]",
name = 93,
options = {},
tick = 43223
}
Received Creation Event: {
created_entity = "[LuaEntity: hps__ml-express-miniloader at [gps=18.5,-1.5]]",
define_name = "script_raised_built",
entity = "SERPENT PLACEHOLDER",
name = 92,
options = {},
tick = 43223
}

It fires a deletion event for the fast miniloader first and then a creation event for the express miniloader. This replaces it correctly.

I implemented your suggestion and added an on_object_destroyed handler for the main object. What happens now is that I see these events:

Received Creation Event: {
created_entity = "[LuaEntity: hps__ml-express-miniloader at [gps=19.5,-2.5]]",
define_name = "script_raised_built",
entity = "SERPENT PLACEHOLDER",
name = 92,
options = {},
tick = 82553
}
Received Object Destroyed: {
define_name = "on_object_destroyed",
name = 178,
options = {},
registration_number = 24,
tick = 82553,
type = 1,
useful_id = 248
}

So the fast miniloader is created first and then the game fires a object_destroyed event for the old loader (id 248). So they arrive in the wrong order. by the time the old loader is destroyed, the new one was already constructed.

I did bang my head against this for a while (see https://forums.factorio.com/viewtopic.php?t=133504) but in the end, the only way to make upgrades work reliably is to use LuaEntity#apply_upgrade(). I guess that is what it exists for.

Do with that information what you want; I explored your advice (using the on_object_destroyed handler) and that does not work. I don't use your mod and I try to help people that use your mod and the miniloader.

Sorry to hear that it didn't work, but what's important about receiving the events in order? Creation and deletion are two separate things that need to be handled, I can't imagine why they'd interfere unless you're doing something with transferring entity settings on the hidden entities. Even in that case, having settings reset is a much better outcome than having a broken entity.

You don't need to tell me about apply_upgrade again, I'm already well aware of the method. I cannot use it as it is the wrong tool for the job.
I'm not trying to cheatily make the entity instantly upgrade, I'm intentionally building a new entity on top of the old one, that way items can spill on the ground automatically.

apply_upgrade voids items any time there is an inventory that gets shrunk, such as downgrading a chest, a machine with less module slots, etc. It also voids items in crafting machines when the resulting machine can't craft the recipe that was selected by the previous machine.
Working around any of that would be a major pain in the ass, I'd somehow have to figure out exactly which items would be voided before the upgrade, and not only is that a bit of a performance hit, it sounds like a minefield for more bugs in the future. I was there when the method was added, and actively chose not to use it for this reason.

There is an unspoken assumption that you can construct an entity on top of another using fast-replace (fast-replace = true in create_entity; If true, building will attempt to simulate fast-replace building). the miniloader main entity (the inserter which is what you see and manipulate) allows that but a creation event requires it to generate its internal state (which is additional inserters and the actual loader). Those can not be stacked (they collide with each other and because they are not built as "fast-replace" (the mod internally just uses create_entity) and that is why the actual construction fails. There is no magic or transfer entity settings or anything; the miniloaders rely on the upgrading of the main entity (all the other entities are synced to the state of the main inserter).

The order matters because "delete, then create" creates the space first, while "create, then delete" requires two entities to (at least temporarily) occupy the same space.

As I said before, I am very open to fix this on my side if I could see a way on how to do this. For compound entities that consist of multiple internal entities, this is tricky and at least I have not found a good way to do it. I believe it is a actually a subtle bug in factorio itself (the create_entity should, when fast replacing, raise script_raised_destroyed for the entity it is fast-replacing) but no one jumped on the bug report/question I posted in the forums.

If there is no easy fix for this, I'd be happy if you could just blacklist miniloaders so that the blueprint shotgun doesn't try to handle upgrading them at all.

Most entities can be script created inside of each other even with collision, but in this case I'm guessing it's the loader specifically that can't be created because anything belt related cannot be overlapping due to transport line merging logic. Would it be possible to have a little bit of extra logic for checking if a miniloader exists in the same position during the creation events, and if so delete it beforehand? That gets the old loader out of the way, and I can't imagine any other scenario where a loader would be created in the same spot as another without a prior destruction event from normal sources.

You are correct, the loader can not be made to "not collide" because it needs to connect to a belt.

I actually posted a "real" bug report on the forums: https://forums.factorio.com/viewtopic.php?t=133860. Anyone chiming in there would be welcome. The "cleanest" fix would be the game engine raising the script_raised_destroy event when fast-replacing. This would instantly fix all the problems.

I was thinking quite a bit about the problem when it was raised to me with Blueprint Sandboxes and I found no good answer. The code could inspect the placement location for existing entities but it would be heuristics and really messy in the code. I will think about it a bit more.