about player.ship.massLockable (new in 1.85)

[Astrobe]

The implementation seems to allow the delegation of the "mass-unlocking" to a script, but it doesn't seem to provide a way to force a masslock. I was looking into this precisely because I was considering having asteroids to masslock players, or ever have a planetary masslock that extends to the WP (with compensations like fuel-free injectors with downsides, but that's another topic).

I'd like to suggest to implement the feature offered by the "variable masslock OXP" directly, that is give every entity a masslock radius; perhaps with special values like 0=never masslock and -1=always masslock at scanner range. If the property is read/write it should allow other use cases (including tricks like setting the player's ship masslock radius to -1 in order to force a permanent masslock).
It seems to me that it could also simplify the code with the right defaults for special cases (beacons, planets, asteroids, ...).

BTW I'm not too familiar with Objective-C and I'm wondering why do updateAlertConditionForNearbyEntities() and loseTargetStatus() make a local copy of uni_entities[]?

[another_commander]

BTW I'm not too familiar with Objective-C and I'm wondering why do updateAlertConditionForNearbyEntities() and loseTargetStatus() make a local copy of uni_entities[]?

Looking at it, it might be that there is one local copy too many (edit after looking some more - I think it's correct as-is). However, note that the code you see here is the result of more than eight years of revisions and at least one of the two cases you mention was located in the HeadUpDisplay class in the past and was transferred over to PlayerEntity at some point. Back then, there could be indeed a very valid reason for doing it this way, which may or may not be applicable now. In fact, there is a comment in the old HeadUpDisplay code (rev. 3f091ec), just when it is about to set the uni_entities, saying: "// use a non-mutable copy so this can't be changed under us." so that must have been the reason back then and the code survived in this form until now.

[Astrobe]

I guess I could try to do the optimization in my local copy and see what happens. Except I smell thread issues (*) so that the fact in works fine here and now doesn't mean it is bug-free. And the optimization may not be worth the risk. I'm know the difficulties of maintaining "legacy" code, I'm in a similar situation at $dayjob.


(*) If my guess is correct then the copy loops should be in some sort of critical section though.

[Astrobe]

Looking at it, it might be that there is one local copy too many (edit after looking some more - I think it's correct as-is). However, note that the code you see here is the result of more than eight years of revisions and at least one of the two cases you mention was located in the HeadUpDisplay class in the past and was transferred over to PlayerEntity at some point. Back then, there could be indeed a very valid reason for doing it this way, which may or may not be applicable now. In fact, there is a comment in the old HeadUpDisplay code (rev. 3f091ec), just when it is about to set the uni_entities, saying: "// use a non-mutable copy so this can't be changed under us." so that must have been the reason back then and the code survived in this form until now.

About my initial proposal: perhaps less heavy-handed would be to make "massLockable" an integer that can take three values:
0 -> no opinion
positive -> forced masslock
negative -> forced "mass-unlock"

So something like:

Code: Select all

// before scanned entities loop
bool ms=[self masslockable];
bool massLocked=ms>0;
...
// inside of the scanned entities loop
massLocked|=(ms>=0)&&[self checkEntityForPudding etc.]; // if ms<0 massLocked will keep its initial value (false)

Aside from my "planetary masslock" personal agenda, I believe other OXPs could benefit from being able to force a masslock like for instance "breakable Torus drive" which had to use quite dirty tricks in order to do that.

Would you accept a patch that does that?

[another_commander]

Sorry for repsonding late but RL is quite restless these days. I would not mind having this modified as you suggest, but I think it needs some adjustments to make it more consistent with what we have for other properties. Rather than havin a massLockable property that takes -1, 0 and +1 as input, it would probably be best to rename it to something like massLockedBehaviour and use descriptive string constants for input. So I imagine it would be something like

Code: Select all

massLockedBehaviour = "OO_MASSLOCKED_NEVER";	// same as massLockable = NO
massLockedBehaviour = "OO_MASSLOCKED_STANDARD";
massLockedBehaviour = "OO_MASSLOCKED_ALWAYS";

[Astrobe]

Sorry for repsonding late but RL is quite restless these days. I would not mind having this modified as you suggest, but I think it needs some adjustments to make it more consistent with what we have for other properties. Rather than havin a massLockable property that takes -1, 0 and +1 as input, it would probably be best to rename it to something like massLockedBehaviour and use descriptive string constants for input. So I imagine it would be something like

Code: Select all

massLockedBehaviour = "OO_MASSLOCKED_NEVER";	// same as massLockable = NO
massLockedBehaviour = "OO_MASSLOCKED_STANDARD";
massLockedBehaviour = "OO_MASSLOCKED_ALWAYS";

Patch available
Also a test OXZ (weapons offline: massunlock, injectors engaged: permasslock)

This is an early patch for review.
Tested:
- shipdata.plist: 3 values for the key + default value
- massLockable property: write
Not tested (spotted and fixed while making the final patch):
- massLockable property: read

I've just realized I forgot to rename the key as you suggested. There may be other things to fix, so I'm waiting for your feedback for now.

[another_commander]

Thanks, will take a look at it once I get a chance.

[Astrobe]

Thanks, will take a look at it once I get a chance.

- [ ] Still too busy to have a look at it,
- [ ] Forgot about it,
- [ ] Asked Linus Torvalds for advice on how to tactfully reject a low-quality patch.

[another_commander]

Can I select more than one?

I did have a look at it at some point and then RL hit again and forgot it. It worked, but I was not very happy with the implementation because I think it is quite different to what we have for other enumeration-to-string situations (e.g. compass mode). I would prefer a similar implementation to that for reasons of consistency, that's all.

[Astrobe]

So all three then I'll take a look at how it's done with the compass.

[Redspear]

5,000 posts!

Congrats/Commiserations