wrong arrival altitude in Map Labels

[bomilkar]

XCSoar versions having the problem

The issue was introduced in 7.34

XCSoar versions not having the problem

Up to 7.33 it worked OK.

System information

Not platform dependent.

Steps to reproduce the behavior

Start XCSoar in Sim mode.
Increase the altitude by clicking on an altitude InfoBox until you see a Waypoint in reach.
Then change Wind manually and observe how the content of "WP AltD" InfoBox changes compared to the altitude shown on the Map Label.

Expected behavior

Up to 7.33 "WP AltD" InfoBox and Map Labels are in sync.

Actual behavior

Do you have any idea what may have caused this?

It seems as if since 7.33 there is a sign error in calculating the arrival altitude in the map label.

	InfoBox	Map Label
head Wind	422	469
no Wind	448	447
tail Wind	470	421

Do you have an idea how to solve the issue?

[lordfolken]

Polar, saftey mc, and stf factor are the same?
We need a unit test for this.

[lordfolken]

Probably all the same by judging the wp altd.

[bomilkar]

Polar, saftey mc, and stf factor are the same.
This becomes obvious at 0 wind: wp altd and altitude in label are the same.

[bomilkar]

We need a unit test for this.

What does that mean?

[bomilkar]

The fix might be trivial, but I simply can't find the part in the code, which calculates the arrival altitude in the map labels.
So unless somebody helps me find it, I won't be able to fix it.

[lordfolken]

So the labels show you the altitude you have above glide ratio at mc 0.
The infobox will factor in saftey mc (from settings) and current mc. If you set this to 0 then its the same.

But as soon as you add the wind component, it increases where its supposed to decrease.

The reachability is calculated here:

(

XCSoar/src/Renderer/WaypointRenderer.cpp

Line 107 in b1244d8

void CalculateReachability(const ProtectedRoutePlanner &route_planner,

)

[bomilkar]

I don't want to give up hunting the bug, but I don't want to predict by when I'll be done. The code is too convoluted for my limited tool set and my abilities. Sorry!
So, I'd like to ask someone else to step in. @lordfolken maybe you??
The bug is too severe (in my opinion) to leave it there for much longer. (However, I'm surprised nobody noticed it much sooner!)

[lordfolken]

Dont worry, im angry at this bug.
"Tactical glide computer" my ass. We cant even get the basics right.
And I also invested last Sunday into this issue. The ammount of abstraction layers in xcsoar are staggering.

[ryanetz]

Just want to thank you guys for taking this issue seriously. I don’t have the right technical skills to fix this, but my hunch is that somewhere in the label arrival altitude calculation a + got replace with a - in the wind factor calculation.
Looking forward for the fix. Meanwhile I ignore the green labels with wind more than 5 knots…

Ramy (who initially reported this issue)

[lordfolken]

So the commit that introduces the bug is: e020359
There the GetSpeedUps is refactored, and tries to reuse the GetDelta() function.
Unfortunatly the arguments for the FlatGeoPoints are reversed in GetDelta, than to what was before.

XCSoar/src/Engine/Route/RouteLink.hpp

Lines 72 to 73 in e020359

	constexpr FlatGeoPoint GetDelta() const noexcept {
	return FlatGeoPoint(second) - FlatGeoPoint(first);

While one could flip those first/second arguments... GetDelta is much older, and was introduced in 2015: ca1ab2c
GetDelta() is used in CrossProduct() and DotProduct() which are functions used all over the place.

So I think its best to revert e020359 and not change GetDelta().