player.replaceShip and Oolite Equipment Control

[dybal]

This one will be a bit hard to explain, so please bear with me...

To come to the conclusions bellow, I put traces in oolite-equipment-control, like this:

Code: Select all

this.equipmentAdded = function(equip)
{
log(this.name,"equipmentAdded "+equip);
    if (!this.$started)
    {
        return;
    }
    if (!this.$equipmentEnabled[equip])
    {
        if (this.$equipmentEnable[equip])
        {
            var info = EquipmentInfo.infoForKey(equip);
//          log(this.name,"Enabling "+info.equipmentKey); //tmp - remove later
            var result = this.$equipmentEnable[equip].bind(this,info)();
            if (result == -1)
            {
                return;
            }
        }
    }
    var eq_dict = this.$equipmentEnabled;
    var eqInfo;
    for (var eqKey in eq_dict) {
        eqInfo = EquipmentInfo.infoForKey(eqKey);
        if (eqInfo && eqInfo.incompatibleEquipment && eqInfo.incompatibleEquipment.indexOf(equip) >= 0) {
            player.ship.removeEquipment(eqKey);
        }
    }
    this.$equipmentEnabled[equip] = 1;
log(this.name,"equipment Added "+equip);
};

this.$equipmentEnable["EQ_SHIELD_BOOSTER"] = this.$equipmentEnable["EQ_NAVAL_SHIELD_BOOSTER"] = function(info)
{
    player.ship.maxForwardShield += parseFloat(info.scriptInfo.oolite_shield_increase);
    player.ship.maxAftShield += parseFloat(info.scriptInfo.oolite_shield_increase);
    player.ship.forwardShieldRechargeRate *= parseFloat(info.scriptInfo.oolite_shield_recharge_multiplier);
    player.ship.aftShieldRechargeRate *= parseFloat(info.scriptInfo.oolite_shield_recharge_multiplier);
    if (player.ship.docked)
    {
        player.ship.aftShield = player.ship.maxAftShield;
        player.ship.forwardShield = player.ship.maxForwardShield;
    }
log(this.name, "Enabled "+info.name+", maxShield: "+player.ship.maxForwardShield+"/"+player.ship.maxAftShield+", shieldRecharge: "+player.ship.forwardShieldRechargeRate+"/"+player.ship.aftShieldRechargeRate);
};

When a savegame is loaded, the world script equipmentAdded event handlers (from all OXPs that define them, and from Oolite Equipment Control) are called for each equipment in the extra_equipment dictionary, but Oolite Equipment Control's break out at the beginning as its this.$started is still false - its $equipmentEnable isn't called for the equipment added, so if the equipment is Shield Booster, maxShields stays 128.

Then comes the world script startUp event, and Oolite Equipment Control's calls its playerBoughNewShip function, that sets its this.$started to true and calls its equipmentAdded function to all installed equipments - then, if Shield Booster is installed, maxShield will go from 128 to 256.

This is the normal and expected behaviour, so far so good!

When player.replaceShip is called, the whole process is repeated, BUT now Oolite Equipment Control's this.$started is true, so the call to its equipmentAdded event handler doesn't return at the begin, but goes on to call its equipmentEnable, and that first pass will change maxShield from 128 to 256 if there's a Shield Booster installed... afterwards, when playerBoughtNewShip function is called, Oolite Equipment Control's will again call its equipmentEnable for all equipments installed, and that Shield Booster will get maxShield from 256 to 384!

Try player.replaceShip("ferdelance-player") on DebugConsole to reproduce it.

So I would suggest adding to oolite-equipment-control.js:

Code: Select all

this.playerWIllReplaceShip = function() {
	this.$started = false;
}

[another_commander]

Was about to post that your proposed fix doesn't work and then I noticed the misspelling: this.playerWIllReplaceShip (see it? )

OK the fix seems to do its thing, but I believe the real reason for what you are witnessing is that the this.$EquipmentEnabled array is initialized to empty inside the equipmentAdded function and it is reset to empty every time this function is called. A simpler fix (which I have not really tested for reasons I'll mention further down), is to move this initialization out of the function and have it as a script global at the top.

In any case, very careful testing of the behaviour of all other equipment types would be needed to confirm no side effects on other equipment appear.

The reason I didn't test much is that I gave it some thought and realized that this fix would end up putting the player in a worse position than before. Shield boosters after ship replacing would end up giving less shields than before. Yes, it would not be consistent with buying the same ship and all, but it's such a small inconsistency in the overall scheme of things, that it was not even noticed all these years till now. Given how difficult the game already is, I was wondering, what if we were to call the current behaviour a feature, not a bug - an easter egg if you will and leave it as is? Give, for once, a slight advantage to the player by keeping a known little bug in? We can even enter a comment in the script to that effect, just so that others do not get confused.

[dybal]

Was about to post that your proposed fix doesn't work and then I noticed the misspelling: this.playerWIllReplaceShip (see it? )

Fingers faster than brain strike again!

OK the fix seems to do its thing, but I believe the real reason for what you are witnessing is that the this.$EquipmentEnabled array is initialized to empty inside the equipmentAdded function and it is reset to empty every time this function is called. A simpler fix (which I have not really tested for reasons I'll mention further down), is to move this initialization out of the function and have it as a script global at the top.

If that's the only alteration I see 4 cases (focusing on Shield Booster, but could be Energy Unit too, civilian or Naval)

- old ship had no SB, new ship has SB as standard -> start replace, SB is added by core game and enabled at equipmentAdded handler, properties reset, no reset at playerBoughNewShip handler so it's already enabled, maxShield: 256, OK (today it would get maxShield: 384, NOK)
- old ship has no SB, new ship has no SB standard -> start replace, no enabling is executed, maxShield: 128, OK (today maxShield: 128, OK)
- old ship has SB, new ship has SB standard -> start replace, SB is added by core game but already enabled at equipmentAdded (nothing is done), properties reset, no reset at playerBoughNewShip handler so SB is already enabled, maxShield: 128, NOK (today maxShield: 256, OK)
- old ship has SB, new ship has no SB standard -> start replace, no SB is added by core game, properties reset, no reset at playerBoughNewShip handler but as there is no SB in the ship nothing is done and SB enabled state has no consequence (unless any spurios ship.removeEquipment(SB) latter triggers equipmentRemoved event, I don't know if it's possible), maxShield:128 OK (today maxShield: 128, OK)

It would only be changing when a problem occurs, but this time against the player.

Now this might work, but it isn't simple:
- removing the this.$EquipmentEnabled reset from Oolite Equipment Control's (OEC from now on) playerBoughtNewShip
- removing the loop calling equipmentAdded for the equipment in the ship at OEC's playerBoughtNewShip
- removing the test for this.$started ifn OEC's equimentAdded and equimentRemoved - OEC's this.$started could be removed as well
- letting the code that deals with equipment incompatibility in OEC's equipmentAdded deal with the incompatibilities when the core game loads a savefile (if this works, the core game code could be simplified)

OEC's this.$EquipmentEnabled would still have to be reset when the ship is reset by the native code (properties reset to shipdata values and no equipments, which I infer occurs when the player buys a new ship and in replaceShip), which I think would have to be done at handlers for playerWillBuyNewShip and playerWillReplaceShip.

The reason I didn't test much is that I gave it some thought and realized that this fix would end up putting the player in a worse position than before. Shield boosters after ship replacing would end up giving less shields than before. Yes, it would not be consistent with buying the same ship and all, but it's such a small inconsistency in the overall scheme of things, that it was not even noticed all these years till now. Given how difficult the game already is, I was wondering, what if we were to call the current behaviour a feature, not a bug - an easter egg if you will and leave it as is? Give, for once, a slight advantage to the player by keeping a known little bug in? We can even enter a comment in the script to that effect, just so that others do not get confused.

I agree it's no big deal. The advantage is short lived, it goes away at the first save/reload. It could affect shield strength, shield recharge, energy recharge and heat insulation...

Few people would notice it happened: absolute (the actual numeric values) shield strength is visible in Numeric HUD and (as in how much stronger than standard) in Combaf MFD (it's how I noticed it), only experienced players would notice something is off with energy and shield recharge (if it's bellow expected, I don't know if I would notice if it's above), heat insulation isn't visible anywhere outside querying with DebugConsole. ASC would be noticed by anyone for sure!

And Shield Booster (civilian or naval), Energy Unit (civilian or naval), Heat Shielding or ASC would have to be in the standard_equipments for the new ship for it to happen.

This bug is triggered not only on replaceShip, but when the player buys a ship with those equipments standard from a ship without.