Oolite Bulletins

For information and discussion about Oolite.
It is currently Fri Nov 17, 2017 9:07 pm

All times are UTC




Post new topic  Reply to topic  [ 11 posts ] 
Author Message
PostPosted: Tue Sep 12, 2017 2:30 pm 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
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[]?


Top
   
PostPosted: Wed Sep 13, 2017 2:14 pm 
Offline
Quite Grand Sub-Admiral
Quite Grand Sub-Admiral

Joined: Wed Feb 28, 2007 7:54 am
Posts: 5011
Quote:
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.


Top
   
PostPosted: Thu Sep 14, 2017 12:54 pm 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
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.


Top
   
PostPosted: Fri Sep 29, 2017 4:05 pm 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
Quote:
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:
// 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?


Top
   
PostPosted: Sat Sep 30, 2017 6:40 pm 
Offline
Quite Grand Sub-Admiral
Quite Grand Sub-Admiral

Joined: Wed Feb 28, 2007 7:54 am
Posts: 5011
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:
massLockedBehaviour = "OO_MASSLOCKED_NEVER";	// same as massLockable = NO
massLockedBehaviour = "OO_MASSLOCKED_STANDARD";
massLockedBehaviour = "OO_MASSLOCKED_ALWAYS";


Top
   
PostPosted: Sat Oct 07, 2017 11:59 am 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
Quote:
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:
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.


Top
   
PostPosted: Sat Oct 07, 2017 1:50 pm 
Offline
Quite Grand Sub-Admiral
Quite Grand Sub-Admiral

Joined: Wed Feb 28, 2007 7:54 am
Posts: 5011
Thanks, will take a look at it once I get a chance.


Top
   
PostPosted: Sun Nov 05, 2017 10:07 am 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
Quote:
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.


Top
   
PostPosted: Sun Nov 05, 2017 12:09 pm 
Offline
Quite Grand Sub-Admiral
Quite Grand Sub-Admiral

Joined: Wed Feb 28, 2007 7:54 am
Posts: 5011
:-)

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.


Top
   
PostPosted: Sun Nov 05, 2017 12:22 pm 
Offline
---- E L I T E ----
---- E L I T E ----

Joined: Sun Jul 21, 2013 12:26 pm
Posts: 545
So all three then :-) I'll take a look at how it's done with the compass.


Top
   
PostPosted: Sun Nov 05, 2017 7:22 pm 
Offline
---- E L I T E ----
---- E L I T E ----
User avatar

Joined: Thu Jun 20, 2013 10:22 pm
Posts: 1105
Quote:
:-)
5,000 posts!

Congrats/Commiserations :D

_________________
"With our thoughts, we make the world" :-)


Top
   
Display posts from previous:  Sort by  
Post new topic  Reply to topic  [ 11 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 7 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Search for:
cron
Powered by phpBB® Forum Software © phpBB Limited