[2.0.23] LuaEntity minable always reads back as false

[haih_ys]

The LuaEntity member minable seems to always read back as false.

To reproduce, put in control.lua the below. Place a car, confirm it is minable, now get in the car. The assert will fail despite the car being minable and it being explicitly set to true

Code: Select all

script.on_event(defines.events.on_tick, function(e)
    for index, player in pairs(game.connected_players) do
        if player and player.driving and player.vehicle then
            player.vehicle.minable = true
            assert(player.vehicle.minable)
        end
    end
end)

[robot256]

Vehicles are generally not minable when players are inside, but I also didn't think the minable property to dynamically reflect that. It appears that reading vehicle.minable actually returns (minable==true AND occupied==false). You can still set the minable property while riding, but you can't read it back without also reading the driving state

1. Read the state of the vehicle with "/c game.print(game.player.selected)" and it prints true.
2. Enter the vehicle with the character.
3. Read the state of the vehicle with "/c game.print(game.player.selected)" and it prints false because minable is true but player is driving.
4. Force the minable state to false by command with "/c game.player.selected.minable = false"
5. Exit the vehicle.
3. Read the state of the vehicle with "/c game.print(game.player.selected)" and it prints false because minable is false.

[Lou]

Thank you for the report.

Well, there is currently a disconnect between lua entity minable read and write. minable write (like setting minable = something via command) changes the entity minability flag. The read also has some mineability logic on top of reading the flag, namely for driven vehicles, and rail elements with dependent entities (trains, elevated rails etc.). What would be the read equivalent of lua write we call internally as isMinableBasic.

While this is unfortunate, I don't think fixing this is worth breaking existing mods.

We could expose the read of equivalent of isMinableBasic, that would just read the flag. If that would be useful to you, please create a feature request topic for it.

Marking this as Won't fix

[robot256]

Thank you for identifying this as a disconnect in the API. This actually explains many cases of mysteriously-unminable vehicles when using my mods which naively read and write the minable property of vehicles that might have players driving them. I understand now your intent is that reading the minable property would reveal whether or not a mine() command would succeed. Personally, I had assumed that the minable property returned the same value that was written, and that you would use the return value of mine() to see if anything else was blocking it. We could check to see if any mods actually use minable the other way, or if they all made the same assumption I did. It's definitely suboptimal to have a data member with different read and write behaviors, rather than separate get() and set() methods with their own definitions.

[Xorimuth]

I would also prefer that this 2.0 change be reverted and a separate 'would mine() succeed' API be added instead if need be. I have some code which needs to check to see if a mod has specifically set a vehicle to be un-minable, but I can't detect the difference between a mod having set `vehicle.minable = false` and there simply being a character in the vehicle. I had to remove the `minable` check when porting to 2.0 (https://github.com/tburrows13/Spidertro ... c1ef94611b), which means that I'm no longer checking to see if a mod marked the vehicle as 'should not be able to be mined' at runtime. Not run into any mod compat issues yet, but it could happen.

[robot256]

While this is unfortunate, I don't think fixing this is worth breaking existing mods.

As Xorimuth noted, and I just confirmed, this behavior is NEW in 2.0. In 1.1, minable reads true when a player occupies a locomotive, for example. So this change is what broke the existing mods, not the other way around. Reverting it to the 1.1 behavior (indicating the state of the isMinableBasic flag, rather than the expected result of a mine() call) would fix mods rather than break them.

[curiosity]

Moreso, as Xorimuth has also noted, this is a degradation of the API since there is no longer a way to obtain the same information. This extra information should have been exposed separately, not tacked onto an existing already readable property.

We could expose the read of equivalent of isMinableBasic, that would just read the flag. If that would be useful to you, please create a feature request topic for it.

What kind of a malicious suggestion is this? You propose to create more confusion in the API. It is natural to expect a property behave like a variable: read and write accessing the same data. If this was the case of the data not being preserved, fine. But you explicitly point out that it is preserved. Imagine if every property was as you suggest: property read and write operate on unrelated data, reading and writing the same underlying data is done via unrelated properties. Using such a system would be mind-boggling.

[boskid]

This read was always returning a value that an entity could override, in 1.1 this was observable in case of a rail under rolling stock which would report as not minable regardless of minable flag set by script on the entity. If there was a reason to make vehicle reports as not minable then it was made to report as not minable the same way as rails were reporting as being not minable. Implementation of LuaEntity::minable is functionally the same as it was introduced in 0.7.1 about 11 years ago.

@curiosity if you want to focus on throwing "malicious suggestion" i can lock this topic already and noone will win.

I can add a separate read that exposes the raw flag but i would need to come up with extra name so there will be ability to read the "minability" and to read the "raw minable flag" separately. If the raw flag would be to be exposed under "minable" i would have to know the scope of how much breaking change it would be, i would rather prefer to have a new variable with read/write where write would effectively overlap with existing minable's write but read would return raw flag.

Under the same change i could also straighten up other operations like LuaEntity's "active" which on write causes disabling by control behavior but when read reports if the entity is active taking all the active flags (disabled by script, disabled by control behavior, disabled by to deconstruct, disabled by recipe, disabled by freezing, and current wakeup state).

[curiosity]

i would rather prefer to have a new variable with read/write where write would effectively overlap with existing minable's write but read would return raw flag.

This is acceptable.

[boskid]

Scripting:
- Added LuaEntity::minable_flag read/write. Write to LuaEntity::minable is now deprecated.

[robot256]

Scripting:
- Added LuaEntity::minable_flag read/write. Write to LuaEntity::minable is now deprecated.

Thank you!