[patch] heading calculation reversed longitudes

[kolesar]

LatLon.heading() returned a heading where degree increased counterclockwise instead of clockwise. Formula used reversed longitude values. See ​http://stackoverflow.com/questions/9457988/bearing-from-one-coordinate-to-another

Fixed, improved readability and performance by calling less toRadians.

[bastiK]

Replying to kolesar:

Formula used reversed longitude values.
Fixed [...].

What about the code that currently uses this method in JOSM core and plugins? Consistency with EastNorth.heading?

[kolesar]

You are right, code using this was previously adapted to the bug. ColorScale.createCyclicScale reverted heading angle back. New patch removes unnecessary reverting of heading angle in createCyclicScale. Also added test case in LatLonTest.

EastNorth uses the same clockwise heading starting from north.

[bastiK]

Replying to kolesar:

You are right, code using this was previously adapted to the bug.

It's not a bug, but a convention - even though an uncommon one.

ColorScale.createCyclicScale reverted heading angle back. New patch removes unnecessary reverting of heading angle in createCyclicScale.

Still the method is used by plugins like photoadjust and simplifyarea. Possibly by unpublished ones as well. Flipping this sign will make them do stupid things. It is better to break the plugin completely (Exception thrown) or not at all.
What we can do is deprecate this method and add a new one, called for example bearing. This might even be a more fitting name for this method.

Also added test case in LatLonTest.

Great!

EastNorth uses the same clockwise heading starting from north.

Okay.

[kolesar]

You are right again, I have forgotten that Eclipse search covers only core, not plugins.

Plugins also reverse back the non-conventional heading from LatLon:

PhotoAdjustWorker.changeDirection: heading is substracted from 360 degrees to reverse back.

    double direction = 360.0 - photoLL.heading(mouseLL) * 360.0 / 2.0 / Math.PI;

iodb.OffsetDialogButton.drawArrow: dx is substracted here from cx to rotate clockwise. This code draws to screen where positive x goes right and positive y goes down. 0 degree means north, cosine is 1, dy needs to be substracted. 90 degree means normally east, this should be added to x. Here it is substracted to compensate counterclockwise rotating heading.

    int dx = (int)Math.round(Math.sin(direction) * length / 2);
    int dy = (int)Math.round(Math.cos(direction) * length / 2);
    g.drawLine(cx - dx, cy - dy, cx + dx, cy + dy);

iodb.OffsetDialogButton.DirectionIcon.updateIcon: to and from are reversed at heading calculation.

    distance = from.greatCircleDistance(to);
    direction = to.heading(from);

SimplifyArea uses its own heading method, change of core does not affect this plugin.

Will you accept a patch that fixes affected plugins?

[bastiK]

The plugin Nanolog uses this method as well. More likely than not, it is also used in some plugin which is not in the main repository. There are quite a lot of those. We do not have a stable plugin-API. In fact, plugins are broken quite often. But at least it is always obvious when a plugin is broken, i.e. some kind of exception is thrown. LatLon is one of the most fundamental classes and you suggest to change the semantics of an existing method. This is much more sneaky and causes more or less subtle strange behavior. This is asking for trouble. As a plugin author, I would be upset if my plugin was broken this way. Besides, there is nothing wrong with the method as it is now. It simply uses a convention that you don't like / expect. Of course, this should be documented more clearly.
As stated above, you can add a new method which returns the clockwise angle and deprecate the current one.

[kolesar]

Replying to bastiK:

It simply uses a convention that you don't like / expect.

Not only me. The word heading means an angle starting from 0 degrees that means North, rotating clockwise, 90 degrees means East. JOSM itself uses this convention in EastNorth.heading method. Do an image search on heading, see that angles are meant clockwise everywhere.

You have asked me to introduce a new method under a new name. This is not trivial because suggested name bearing means totally different from heading in navigation, but return values of old heading and new bearing methods are different only by mirroring.

When we calculate an angle from starting LatLon to another LatLon, it is meant to be an angle at starting point between north and great circle path between points. This angle is called true bearing. Word true means it is calculated from north. Relative bearing means angle between your current heading and the destination. What is heading then?

Heading means the direction of your nose, vehicle, ship, etc. If you turn your heading to the true bearing and move straight (=on great circle path), you will reach your destination. This assumes you always move in the direction of your heading. But movement of an aircraft or ship can be different because of wind in air or current in sea. That's the difference of bearing and heading.

JOSM uses heading method for two purposes.

• heading: calculate your assumed heading at a point of track path
• bearing: calculate bearing from a point to another

As track files usually don't have information for true heading, we don't know if that movement was affected by wind or current, therefore we assume heading equals to bearing from last track point to next. The angle we calculate from two LatLon values is always a (true) bearing.

Therefore most appropriate name for the corrected method would be bearing. Calls to old method should be eliminated shortly because name heading is false, it is bearingCounterClockwise.

Reversed angle is often tried to reverse in a bad way. Image offset database and NanoLog plugins turned angle back using flipping from and to points. It works only for very small distances. Bearing from A to B is not equal to 360 - (bearing from B to A), except when latitudes are equal. Bearing to destination on great circle path varies continuously except if you move on Equator (90 or 270) or a Meridian (0 or 180).

Conclusion: current method has a wrong name and returns angle in an unusual direction. This makes trouble when it is tried to be reversed by flipping arguments.

It's a good opportunity for introducing the correct calculation under the correct name. I will create patch using this way.

[kolesar]

Created bearing method, marked heading deprecated. Replaced LatLon.heading calls with bearing in core.

Fixed fast drawing of direction arrows in GpxDrawHelper that was already broken in tested version. It drawed absolutely false arrow heads. It had a list of cached arrows with 12 items where last 4 items were same as first 4, this means 8 unique directions in 45 degree steps. Code casted heading angle (radians) to integer directly (without scaling) and used this value as an index of this array. Radians are between 0 and 2*PI (6.28), therefore indexes 0..6 were only used, index 7 never. Moreover, angles were rotating in the opposite direction. You can easily test it by drawing a circle and converting to GPX layer.

Attached patch file. I have trimmed arrowhead cache to 8 unique values, fixed scaling and angles.

[bastiK]

In 9796/josm:

applied #12524 - heading calculation reversed longitudes (patch by kolesar)

[bastiK]

Replying to kolesar:

It's a good opportunity for introducing the correct calculation under the correct name. I will create patch using this way.

Great!

Replying to kolesar:

Fixed fast drawing of direction arrows in GpxDrawHelper that was already broken in tested version. It drawed absolutely false arrow heads.

For me, this shows the disadvantage of non-trivial features that are only available as advanced preferences. It is essentially dead code that breaks easily and still requires valuable developer time.

[bastiK]

GpxDrawHelperTest fails.

[kolesar]

Output of GpxDrawHelperTest is not related to this patch. It says:

configured server url 'https://api.openstreetmap.org/api/' seems to be a productive url, aborting.

I have modified url to developer api but the message was the same.

Used the following command: ant clean test. Is something wrong here?

I can't run this test from Eclipse, complains about preferences file and can not find resources. I can run or debug JOSM from Eclipse but tests fail.

Failed to open input stream for resource 'resource://data/preferences.xsd'

[bastiK]

In 9806/josm:

see #12524 - forgot to commit tests (patch by kolesar)

[bastiK]

Replying to kolesar:

Output of GpxDrawHelperTest is not related to this patch.

You can see the output here.

It says:

configured server url 'https://api.openstreetmap.org/api/' seems to be a productive url, aborting.

I have modified url to developer api but the message was the same.

Used the following command: ant clean test. Is something wrong here?

This should work. The task test-init copies test/config/preferences.template.xml to test/config/unit-josm.home/preferences.xml to ensure server config with development server URL. It seems that this didn't work for some reason or the test code picks up wrong preference dir.

I can't run this test from Eclipse, complains about preferences file and can not find resources. I can run or debug JOSM from Eclipse but tests fail.

Failed to open input stream for resource 'resource://data/preferences.xsd'

It should work if you run all the tests using the given test-configuration. If you want to test individual files, you need to add a run-configuration for each and add the bin2 directory to classpath. (Basically copy the config for the working test config.)

[kolesar]

Thank you for your help, I was able to set up working test environment. I had an empty preferences file at test/config/unit-josm.home. Ant task test-init does not overwrite this file if exist and clean does not delete. I think overwrite could be enabled in copy task, do you agree? Or does someone customize this copy of preferences file after initial copy operation?

testDirection failed because it listed old samples of an erroneously reversed color scale. ColorScale.createCyclicScale creates an array of colors indexed from 0 to 255. Loop variable is scaled between 0 to 4, then integer part is substracted to get fractional part.

As LatLon.heading() returns a counterclockwise increasing angle, previous version of color scale reverted scale by substracting this value from 4 (instead of substracting loop variable from 255). Index 0 was computed from 4 instead of 255/256*4. This difference caused a one-step shift in array values. Previous version of code:

float angle = 4 - i / 256f * 4;
int quadrant = (int) angle;
angle -= quadrant;

Index 0 was special because angle was 4, quadrant was 4 and therefore angle became 0. Index 1 to 64 used values from 0.984375 to 0.000000. Index 255 was computed from 0.015625.

Fixed version used clockwise bearing and removed substraction from 4:

float angle = i / 256f * 4;
int quadrant = (int) angle;
angle -= quadrant;

This change reversed the color scale to be suitable for clockwise values. Besides of reversing it shifted the color ring back with one step. Index 0 is computed using fraction 0, index 1 to 63 is computed from 0.015625 to 0.984375. Index 64 uses fraction 0 again, and so on to 255.

Shift means a change of 256/360 = 0.71 degrees. Difference is invisible for eyes but RGB values showed a minor difference that resulted test failure.

Fixed issue by using new values from color scale in test case, attached patch.

[bastiK]

In 9809/josm:

fixed #12524 - update unit test (patch by kolesar)

[bastiK]

Replying to kolesar:

Thank you for your help, I was able to set up working test environment. I had an empty preferences file at test/config/unit-josm.home. Ant task test-init does not overwrite this file if exist and clean does not delete. I think overwrite could be enabled in copy task, do you agree? Or does someone customize this copy of preferences file after initial copy operation?

Not sure. Vincent, what do you think?

[Don-vip]

In 13966/josm:

see #14120, see #12524 - remove deprecated method LatLon.heading()