Fixed suspicious if-expression

[alexeylysenko]

In existing code there is no need to check specifiesCoordinate() and coordinate().isValid(), because they are always true. I assume the correct logic should be like that (start timer if expression is true or set _terrainAltitude to NaN otherwise).

[DonLakeFlyer]

Obviously out of whack. But there is some subtle insanity in this area which I vaguely remember dealing with. Going to take some time for me to get into this to remember exactly what needs to happen.

[DonLakeFlyer]

Part of the mystery is here: e476d52#diff-440910e4f5733cf3d942e2b3afa29237

Still digging...

[alexeylysenko]

Part of the mystery is here: e476d52#diff-440910e4f5733cf3d942e2b3afa29237

Still digging...

Judging by that commit my changes are still correct. They keep the idea "Don't query terrain if no location".

[DonLakeFlyer]

Judging by that commit my changes are still correct.

That is a correct statement, but only if the original logic is correct. Which I'm not sure of at this point.

[DonLakeFlyer]

So the concern is around the case where the user changes the command for an item. This can cause specifiesCoordinate to change to false, but the coordinate will stay valid. The coordinate is not cleared in this case to keep it around in case the user switches back to a coordinate based command. And then given all of the above what happens to the terrain value. Should it be kept, or should it be cleared. The answers to those questions could affect the need for different boolean logic. In looking through that I discovered there is no signalling connected to specifiesCoordinateChanged associated with terrain values. So I needed to also determine whether that was a problem or not.

Anyway, not sure if that makes any sense to anyone who hasn't started at this stuff for months. It a complexity problem caused by the fact that the base class for all items: VIsualMissionItem is trying to do generic work with respect to terrain altitudes so that the superclasses don't need to. But the superclasses control some of the data like the command which can affect the lower level values.

It critical to get this right because if you don't it can lead to strange situations where things are waiting on terrain data when they shouldn't be. And if that happens at the field, where you may hav no internet you can get hosed. Hence my paranoia.

After poking around and staring at it for a while I've decided that:

• It make sense to keep the old terrain altitude for an item even when the item transitions to !specifiesCoordinate
• It's ok that there is missing signalling on specifiesCoordinateChanged since the first bullet above is true

So long story short this change is fine.

[DonLakeFlyer]

And thanks again for the fix