Missile - Target distance

[paccalin]

Resolves #131 . Implement a more formal and less computation-hungry algorithm for the computation of the shortest distance between a missile and it's target.

[NikolaiVChr]

3 of the methods you created already exist.

Check out: minus(), plus() and product()

[NikolaiVChr]

And yeah, they could have been named better, is probably why you missed them.

[paccalin]

3 of the methods you created already exist.

Thank you, I amended my commit.

[paccalin]

Hey guys, I'm having some trouble. I have the core code ready but I am having some trouble with the data: In missile-code.nas, I was expecting me.coord to be the coordinates of the missile at the current frame, and me.last_coord to be it's coordinates at the previous frame (same for me.t_coord and me.last_t_coord but for the target). However, when comes the time to compute the closest approach (function explode(...)), these coordinates are exactly the same: me.last_coord.direct_distance_to(me.coord); gives me 0, same for me.last_t_coord.direct_distance_to(me.t_coord);. @NikolaiVChr, Am I getting something wrong ?

[NikolaiVChr]

Nope your not, something is off. This code is run shortly before explode/proximity_detection:

                # record coords so we can give the latest nearest position for impact.
        me.before_last_coord   = geo.Coord.new(me.last_coord);
        me.last_coord          = geo.Coord.new(me.coord);
        if (me.Tgt != nil) {
            me.before_last_t_coord = geo.Coord.new(me.last_t_coord);
            me.last_t_coord        = geo.Coord.new(me.t_coord);
        }

And in between that code and explode, me.coord is not updated. Which will lead to them being the same.

To be honest I do not remember why that code was moved there, it used to be under proximity detection..

[paccalin]

Thank you for the insight. I'm using this code review opportunity to do some cleaning and refactoring. Although, it would seem that me.coord must be updated at some point since it's value is changing, but it must be outside this callstack.

[NikolaiVChr]

Note that this missile code is being used in about 15 other aircraft. So when you change stuff please do it in incremental commits. So I don't get 1 commit with 400 lines changes I have to review and port over, especially if you are going to refactor stuff. Also because there is 3 versions of this script which makes porting over a little trickier: this version, a version with some inaccuracy system built in used for F16 targetpod and an emesary version (which hopefully soon will become the only version).

[Zaretto]

@paccalin as Nikolai said - please do not refactor just to make the code prettier because this code is being used in lots of aircraft and it makes following the changes very difficult if there is a refactor of names, style (etc).

For refactoring my rules are:

• do not just move lines / methods around
• do not rename methods
• do not re-indent or add whitespace
• do not convert tabs to spaces (or spaces to tabs)

Generally with the (OPRF) shared modules be very careful and change the minimum - because these modules have many hundreds if not thousands of hours of development time from many developers - to get things right; and it is easy to break things without realising it.

Liaising closely with Nikolai before making any changes is highly recommended

[paccalin]

Note that this missile code is being used in about 15 other aircraft.

@Zaretto, @NikolaiVChr Shouldn't we make it a git submodule then ?

So when you change stuff please do it in incremental commits. So I don't get 1 commit with 400 lines changes I have to review and port over, especially if you are going to refactor stuff.

No problem, that's a bad practice to push unspecialized commits anyway !

Generally with the (OPRF) shared modules be very careful and change the minimum - because these modules have many hundreds if not thousands of hours of development time from many developers - to get things right; and it is easy to break things without realising it.

Stopping improvement on something because "there is a lot of work already done on it" is very bad and probably one of the reason why so many models are poorly maintained (if they are maintained at all). Ideally we would have clear conception documents which would undeniably highlight many flaws in the current implementations (not targetting anything specific here).

I will propose what I've been working on (which I personally consider much more precise, formal and efficient (see #132 comments)). Wether you guys want to integrate it here and in other aircraft is up to you ^^

If you want to set-up submodules for generic OPRF scripts, I have some experience in submodules and can help you on this point.

[Zaretto]

Shouldn't we make it a git submodule then ?

Ideally yes; but not all of the aircraft use an identical module and changes have to be merged by hand. I did start a project a few years ago to consolidate down to a single module but it turned out to be a lot more difficult than I imagined and I had to forget the idea.

Stopping improvement on something because "there is a lot of work already done on it" is very bad and probably

Agreed and it is not my intention to stop improvement; the reason that we here as good a system as we do is because of contributions of improvements. Improvement is something that I welcome.

I just get worried when folks start talking about refactoring.

Looking at your commits I'm happy with what you've done; except for a small question mark over using a recursive function inside a real time system - we have to be careful that the recursion is depth limited to avoid a frame rate impact.

[paccalin]

Looking at your commits I'm happy with what you've done; except for a small question mark over using a recursive function inside a real time system - we have to be careful that the recursion is depth limited to avoid a frame rate impact.

@Zaretto Since we are only doing the computation when the range is increasing, the closest range may have happened at last frame (and not current frame). Although the function can (if we give it the proper parameters) recurse on as many frames as we want. I only use it with the defaults parameters (where mfd = m.crc_frames_look_back = 2). Which means that IF (and only if) the closest approach happened in the previous frame. It will compute it.

But since I'm only using it with mfd = 2. It will only do it once (terminal recursion will append after the second call).

Technically this function could have been coded without recursion (given that the terminal limit is a constant) but I did it this way to make it more readable and to avoid lines duplicates (it is kind of the equivalent of making the two loops of the old "interpolation" method into only one that can be called 2 times).

Since the maximum recursion depth is constant (2). I can guarantee that the recursive function won't go over the 2 last frames, which could have been done with the same level of complexity with a non-recustive method (yet, the code would have been less readable and less generic).

So overall, I made this a recursive function mostly for genericity, but the recursive version turned out to also be more readable (IMO).

Edit: There is also someting else that I didn't notice for my first answer. This function isn't part of the real time system loop, but triggered by an event that will happend at most once in the whole missile life-cycle. I don't think this is a sufficient enough reason to not take time to discuss optimisation. But we have to keep in mind that this function is extremely rarely called too. Don't get me wrong, I don't think it is possible to get this function to be much more efficient without changing language. (as I already spent a huge amount of time applying most optimisation techniques I know of), but still ...

[paccalin]

I just get worried when folks start talking about refactoring.

Refactoring is good. It trully allows you to take a step back on the logic of the code, which often result in simpler approach (in terms of performances, readability and maintainability). But be reassured, I won't refactor the whole file. Only the tools I'm replacing or those I need to use. Doing a full refactoring on this file would take a lot of time from a lot of peoples. Time which I don't have.

[paccalin]

@NikolaiVChr , @Zaretto, @5H1N0B11 This is ready for review 😃

[NikolaiVChr]

Alright, by a first glance it looks good.

This line: me.crc_range[i] = (i != 0) ? me.crc_range[i-1] : vector.Math.magnitudeVector(

is not ideal. vector.Math might be in use by another thread, so its not threadsafe to use in that method. So have to use me.myMath which is a copy used only by that missile while its flying, that way avoids using a mutex.

Notice only methods thats meant to be called before firing uses vector.Math

[paccalin]

is not ideal. vector.Math might be in use by another thread, so its not threadsafe to use in that method. So have to use me.myMath which is a copy used only by that missile while its flying, that way avoids using a mutex.

Well I use vector many times (somewhere around 5 calls IIRC), there should be only one thread for each missile, right ? This brings me to a question since me.crc_range[i] (and the other data tables) are relative to the missile and only used in the missile loop (I've just added these variables, so I can be pretty sure of that). There should be no concurrent access to this part of memory (vector or not), thus, no mutual exclusion necessary. Am I right?

The truth is that I actually don't understand why you're bringing the mutial exclusion thing on a class and not on an object (or any instance of anything for that matter), which is usually how we think about mutex(es, does it plural ? 😆) and critical pathes on multi-threaded applications.

[NikolaiVChr]

Well everything in main FG nasal thread uses vector.Math which is a class. However me.myMath is a instance of vector.Math, it has vector.Math as parent. Which means each missile has its own instance. Each missile has its own thread yes, you have 5 in flight, that's 5 threads. Could maybe have been one, but is not at the moment. And yes, you are right.

[paccalin]

Okay, so I've read the nasal doc and some forums on the subject more thoroughly, and I think I started to understand some things. Nasal is actually not really OOP, and there is not really such thing as instance but only static classes inheriting from each other (with static members). So if I understand correctly, the concurrency problem has to do with the fact that these static function members are not pure functions, due to the fact that we use static memory members (me.myVar) even for things that belongs to the responsibility of the scope/function (var myVar inside { }). And we do that for faster runtime (I've been told).

[NikolaiVChr]

The reason for using me.myVariable instead of var myMariable is that me.myVariable does not get garbage collected. Its kept in memory in the scope until next time the function inside the scope uses it again. If use var they get scheduled for garbage collected after each time the function has ran. And garbage collection in Nasal is a performance hog.

So for the concurrency problem, thread A uses vector.Math.Minus(), but gets interrupted in the middle of the function, thread B uses then same function, later thread A continues in that function, but now one of the variables it uses has meantime changed to something completely different due to thread B, and exits with a wrong result. (And yes that actually happened before I realized I had to let each missile have its own vector math library or put mutex'es on it.)

[paccalin]

Okay, I didn't know that Nasal used garbage collection but since it is apparently permissive, this seems quite obvious indeed, and I concieve that for a language that is interpreted only, this can come at a big cost. Otherwise ?

[NikolaiVChr]

Otherwise ?

Sorry. Otherwise what?

[paccalin]

I've added the correction. Any other error you see?

[paccalin]

Maybe @Zaretto might also want to take a look at the subframeClosestRangeCoord(fei, fsi) member (line 3662 in missile-code.nas), Now that it is integrated in the proximity_detection() member (line 3637). With defaults fei = 0 and mfd = me.crc_frames_look_back = 2 parameters.

[NikolaiVChr]

Since it uses frames for time, will it still hold up if the frame-spacing (time between frames) is uneven? As far as I can understand it, it shouldn't matter, just asking to be sure. I cannot see anything more to comment on.

[paccalin]

Since it uses frames for time, will it still hold up if the frame-spacing (time between frames) is uneven? As far as I can understand it, it shouldn't matter, just asking to be sure.

This a very good question: Each range check is done with the speed during the current frame so any dt calculated with the speed and coord of the current frame is only used to interpolate the current frame, so overall the dt in second can be whatever it is since we do not extrapolate the frame. We use the result in a test: < 0 to know if we have to redo the whole thing for the previous frame, but since it will all be computed with the coord and speed of the previous frame (with it's potentially different scale of dt in seconds), this should all be okay too since here again we will not extrapolate and clip dt to [0; 1]

So the thing to take, is that since each (of the 2 maximum) potential computation and interpolation is done independently, and that we do not extrapolate, or exchange values between CRC frames computation, it is safe for a noisy clock.

Obviously, higher clock will always provide higher precision in this implementation since we are not integrating acceleration. But still, I've already managed 0.0m in the most perfect conditions (airliner cruisinng <5nm) with the noisy "slow" nasal clock, so, it seems okay, but this was already a problem previously, and integrating acceleration would be IMO to big of an overhead for the degree of precision it brings at these time scale.

[NikolaiVChr]

Alright awesome. Hope Shinobi will merge it then. :)

[5H1N0B11]

Well done!

[NikolaiVChr]

Wont these lines give errors:

me.explode("Selfdestructed.");

[NikolaiVChr]

Also this line gave me exception due to range being nil: if (range < me.reportDist) {

Its fixable by just adding a nil check, unless it was supposed to never be nil..

[paccalin]

me.explode("Selfdestructed.");

Indeed, I totally forgot to change them, nice catch.

Also this line gave me exception due to range being nil: if (range < me.reportDist) {

Its fixable by just adding a nil check, unless it was supposed to never be nil..

The initial idea was that if the target existed, the range would be provided, but this indeed is an assumption we cannot make from inside the function.

Thank you for the helpfull review. This was very helpfull, but maybe next time we could use the "review" tool in the merge request "changes" page.

This allows to target specific lines / changes, without having to quote them out of context!

I'll push some fixes soon !

[paccalin]

@5H1N0B11 Can you re-open this merge request for me ?

[paccalin]

Will be ready for review once reopened.

[paccalin]

@5H1N0B11 Can you re-open this merge request for me ?

I'm not used to GitHub specificly, apparently you can't re-open a merged PR. I'll open a new one.