Modify

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#12524 closed defect (fixed)

[patch] heading calculation reversed longitudes

Reported by: kolesar Owned by: team
Priority: normal Milestone: 16.02
Component: Core Version: tested
Keywords: Cc:

Description

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.

Attachments (2)

LatLonHeadingFix.patch (6.9 KB ) - added by kolesar 8 years ago.
GpxDrawHelperTest.patch (946 bytes ) - added by kolesar 8 years ago.

Download all attachments as: .zip

Change History (19)

in reply to:  description comment:1 by bastiK, 8 years ago

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?

comment:2 by kolesar, 8 years ago

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.

in reply to:  2 comment:3 by bastiK, 8 years ago

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.

comment:4 by kolesar, 8 years ago

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?

comment:5 by bastiK, 8 years ago

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.

in reply to:  5 ; comment:6 by kolesar, 8 years ago

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.

http://wiki.geocaching.cz/images/c/c1/Azimut.jpg

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).

https://www.mapthematics.com/Essentials/Images/13_Great_circle.png

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.

by kolesar, 8 years ago

Attachment: LatLonHeadingFix.patch added

comment:7 by kolesar, 8 years ago

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.

comment:8 by bastiK, 8 years ago

Resolution: fixed
Status: newclosed

In 9796/josm:

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

in reply to:  6 comment:9 by bastiK, 8 years ago

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.

comment:10 by bastiK, 8 years ago

Milestone: 16.02
Resolution: fixed
Status: closedreopened

GpxDrawHelperTest fails.

comment:11 by kolesar, 8 years ago

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'

comment:12 by bastiK, 8 years ago

In 9806/josm:

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

in reply to:  11 comment:13 by bastiK, 8 years ago

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.)

comment:14 by kolesar, 8 years ago

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.

by kolesar, 8 years ago

Attachment: GpxDrawHelperTest.patch added

comment:15 by bastiK, 8 years ago

Resolution: fixed
Status: reopenedclosed

In 9809/josm:

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

in reply to:  14 comment:16 by bastiK, 8 years ago

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?

comment:17 by Don-vip, 6 years ago

In 13966/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.