Pan & goto with custom events

[elgandoz]

Brief summary of the changes

• New parameter goto for the AddWaypoint event: this creates a temporary waypoint "(goto)" and sets it as navigation target.
• Argument for GotoLookup event: parameter will be looked up within the waypoints names, if provided.

event=GotoLookup (goto)
event=AddWaypoint goto

Further informations

The goal is to provide an easy way for the user to configure an event in order to "Pan & Go" to any point on the map, particularly useful for "improvised" XC, very common in paragliding.

5_6205962494129735712.mp4

Sample of the custom-events.xci used in the above demo:

mode=pan
type=key
data=APP4
# event=Pan off
event=GotoLookup (goto)
event=AddWaypoint goto
event=StatusMessage Goto point on the map
label=Goto\nPoint
location=4

Download here: custom-events.xci.txt
(Remove the .txt at the end)

This functionality is a common pattern in other freeflying computers.

A possible enhancement of this feature would be enabling the "Goto" button (and maybe the first page of Details) in the "Map element at this location" dialog.

SeeYou Navigator	XCTrack	XCSoar (proposed)

Related issues and discussions

• https://groups.google.com/g/xcsoar-user/c/z3_zckhSh1E
• Add Waypoint on Map #509

[elgandoz]

Instead of binding the DoGoto within the addWaypoint event, I extended the GotoLookup event in order to lookup the argument, the waypoint name.
EG: event=GotoLookup Airfield
In this way we can still bind the creation and navigation of the temporary waypoint in a custom event file.

[lordfolken]

The osx ios pipline is a sanity check. The build failure is unrelated.

To the pr. In general the waypoint creation is imo ok.
Although i dislike the string match, and would have used the flags like the home waypoint for selection and deletion. But given takeoff is already implemented this way...

The tie in via inputEvents feels wrong.

Also replacing the whats here button isnt friendly towards joystick users.

I would have tied it in via the

MapItemListWidget::OnGotoClicked() function.

[lordfolken]

@MaxKellermann do you agree?

[elgandoz]

Hey @lordfolken, thank you for the feedback!

Although i dislike the string match, and would have used the flags like the home waypoint for selection and deletion. But given takeoff is already implemented this way...

I agree to that, but I wanted to start by little steps, without modifying too much, by using the existing "takeoff" pattern. Maybe I could try to refactor both later by using flags (like "home")?

The tie in via inputEvents feels wrong.

I used the input event for few reasons:

• it was the pattern I was already using but with markers (using custom events)
• it is the (takeoff) pattern for the gce=TAKEOFF
• it doesn't change any existing behaviour
• it can be used and customised as the user please (for example I added the event=Pan off in order to switch immediately back to the map)
• when set-up like the example, it allows the minimum number of taps/clicks to achieve the feature.
• events can be overridden (for example, removed the auto-marking of the takeoff waypoint since while hang/paragliding often you don't want to land back on top of the mountain where you launched)

Also replacing the whats here button isn't friendly towards joystick users.

It is not, but that's part of the customisation the custom events allow. The change is not enforced by this PR, the sequence posted is just an example on how to use it.
On a paraglider we have both hands occupied on the controls, so I wanted to minimise to the very minimum the number of tap/clicks: replacing the "What's here" was my preference since I can get to the same window by tapping directly on a point. To each their own.

I would have tied it in via the MapItemListWidget::OnGotoClicked() function.

That would be an extension of this ticket, as I sort of mocked it up in the screenshot (although not really connected to this very PR).
I was waiting to hear opinions on this one before starting. That would be the "proper" way, although not customisable as this one. I personally would like to have them both.

[lordfolken]

Hey @elgandoz this was good work. Are you ready for review?

[elgandoz]

Hey @lordfolken, sorry for the long absence.

The PR was ready for review all along.

I now fixed the refactoring conflicts by using data_components.
Could please check if these lines follow the right approach?

• https://github.com/XCSoar/XCSoar/pull/1065/files#diff-db7439a8b995f3f0a1251a530565dee3741b42a6413d0b264d5073a735a1b076R669-R670
• https://github.com/XCSoar/XCSoar/pull/1065/files#diff-82e8497c828b489d7468e692741536dee5c9b0da61ca3e5e4f2a02cf923ee76dR108

Unfortunately Android and Windows don't build, although the issue doesn't seem related with this PR, please advise.

[elgandoz]

I found a bug and fixed it:

• added goto_point.has_elevation = true; introduced in bd61657
• added noexcept introduced in e772c0e

[MaxKellermann]

Your first commit consists almost completely of code you copied. Adding redundant code is not acceptable.

[elgandoz]

Your first commit consists almost completely of code you copied. Adding redundant code is not acceptable.

@MaxKellermann it's obviously the same pattern as the Home waypoint. Do you have at least any suggestion on how it should be done?

[elgandoz]

@MaxKellermann @lordfolken, I rebased from master and adjusted the code in order to not have duplicated code.
Now it generates and adds the "takeoff" and "goto" waypoints from the same function.
Could you please review?

Android and Windows do not build but it doesn't seem related with this PR.

[lordfolken]

This now looks concise.
At some point we are going to have to find a better internal repesentation of waypoint properties, that are not text strings. But this isnt what this pr wants.

@MaxKellermann can we consider merging this?

[MaxKellermann]

@MaxKellermann can we consider merging this?

No, because the overall concept of this implementation is confusing and too complicated.
Look at the example configuration:

event=GotoLookup (goto)
event=AddWaypoint goto

Now explain to me what this does, and why it is written that way. Who, except for the submitter, will ever be able to come up with this configuration?

So .. what this does is (1) add a new waypoint at the current location. How do we know it's the current location? Why does it say "goto" when it's not about "goto" at all? Why doesn't it say "current location" instead? Confusing!

Then (2) it does "goto" to this new waypoint. Uhhh... which waypoint? Not the "goto" waypoint, but an arbitrary waypoint whose name just happens to be "(goto)" (yes, name with parentheses). But ... why that exact name? Because it's an undocumented implementation detail of the current code, which can change any time, for example when somebody decides to translate the name. And what happens if this feature was used twice? Then we have two waypoints with that name. Which one will be used? I have no idea.

This feature is too complicated, confusing, fragile and undocumented.

[elgandoz]

@MaxKellermann

Now explain to me what this does, and why it is written that way. Who, except for the submitter, will ever be able to come up with this configuration?

I don't think it's that different from what currently already happens in the default.xci here and here

So .. what this does is (1) add a new waypoint at the current location. How do we know it's the current location? Why does it say "goto" when it's not about "goto" at all? Why doesn't it say "current location" instead? Confusing!

I am a bit confused by this statement... it creates a waypoint under the sight cross, where the user would like to "go to" (like the event/label name).
I'm not sure what you refer to with "current location", but that would make me think at the current location of the glider.

Then (2) it does "goto" to this new waypoint. Uhhh... which waypoint? Not the "goto" waypoint, but an arbitrary waypoint whose name just happens to be "(goto)" (yes, name with parentheses). But ... why that exact name?

As I already explained in a previous comment, I used that parenthesis pattern only because the same arbitrary name is used for "takeoff" (displayed (takeoff), or better (take using default settings), and I wanted to maintain a consistent behaviour with what's already happening.
I would rather use goto without parenthesis, it's a very easy change, I'm all for it.

Because it's an undocumented implementation detail of the current code, which can change any time, for example when somebody decides to translate the name.

But isn't it the same for (takeoff)? If translated that in their own .xci is not going to work.
Those should be considered special waypoint labels.

And what happens if this feature was used twice? Then we have two waypoints with that name. Which one will be used? I have no idea.

If used twice it removes the previously created waypoint with the same name, just like it happens for (takeoff).
This would happen only for the labels takeoff and goto. If the user uses another arbitrary name a regular waypoint would be created (more than once if it already exist), but I believe that is the current behaviour already.

About the documentation, this is lacking and it took me a moment to understand what the current code is doing (see #509).
The goal of this PR is to improve the Event, by modifying as little as possible of the current behaviour. I would be happy to document the feature, including currently undocumented events, if this is getting accepted.

I previously asked for recommendation on what can be done better but I received none.

[elgandoz]

Hey @lordfolken, I saw you marked this for the next release.

I was wondering if you still want me to:

• rename goto to something better
• as suggested by Max, change the misc parameter name for GenerateTempPoint(). Would name be better?
• uniform the name of the goto waypoint when added by AddTempPoint(), without the parenthesis?
  • I used the paranthesis because that was what was happening for (takeoff) but maybe that's not necessary in this case, simplifying the call in .xci: event=GotoLookup goto, event=AddWaypoint goto

Also, as separate PR, I can have a look into using MapItemListWidget::OnGotoClicked(), the idea for this one was to use the custom action, like someone pointed out in that Google forum post

[lordfolken]

Hey @lordfolken, I saw you marked this for the next release.

Technically its the release after that. The strategy is to stabilize and fix all the current bugs in 7.42. for the moment. I also like the feature's functionality and i think it should be in XCSoar.

I was wondering if you still want me to:

* rename `goto` to something better

* uniform the name of the `goto` waypoint when added by `AddTempPoint()`, without the parenthesis?

The string as such is fine with me to be "goto". I'm also fine with the parentheses, to indicate to the user that this waypoint is generated.

• Its my plan to make the default waypoint name on the map the Shortname, and if not available the 5 first letters of the full name. So (goto) would be fully displayed as long as you write it into the shortname string of the waypoint.

The main problem i see with extending the AddTempPoint() function, is that we currently have no way of differentiating between a user provided waypoint and a generated one. A string match isn't enough to isolate this from the user playload. (this was already a problem with the current implemenation...)

There should be a flag/enum in Waypoint.hpp to indicate that this waypoint is programmatically generated.
Potentially the flag would be saved for each waypoint and, it could be for example an enum. This one should differentiate between files that have been loaded, the user.cup waypoints, takeoff and goto waypoints. This way we can handle all the use cases without string matches.

* as suggested by Max, change the `misc` parameter name for `GenerateTempPoint()`. Would `name` be better?

i'd go for name as this is also in the waypoint struct.

  * I used the parenthesis because that was what  was happening for `(takeoff)` but maybe that's not necessary in this case, simplifying the call in .xci: `event=GotoLookup goto, event=AddWaypoint goto`

Also, as separate PR, I can have a look into using MapItemListWidget::OnGotoClicked(), the idea for this one was to use the custom action, like someone pointed out in that Google forum post

Pan ultimate that should be the goal.

[elgandoz]

Rebased and renamed variable