MGRS conversion fixes #68
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix index out of bounds error due to a typo in convertMGRSToUPS; Fix handling of extreme southern latitude (zone 0) conversions following Geotrans 3.7 for guidance: should give Lat: -89.345400 deg, Lon: -48.930600 deg ==> MGRS: AZN 45208 47747
wcmatthysen
commented
Aug 11, 2019
| j = i; | ||
| while (i < MGRSString.length() && Character.isDigit(MGRSString.charAt(i))) | ||
| while (i < MGRSString.length() && Character.isDigit(' ')) |
There was a problem hiding this comment.
@gbburkhardt, I'm not sure this change is correct. The Character.isDigit(' ') call will always return false.
There was a problem hiding this comment.
I think we can drop this while loop from the look of it.
| * | ||
| * @return the error code. | ||
| */ | ||
| private long checkZone(String MGRSString) |
There was a problem hiding this comment.
@gbburkhardt, is it ok for us to drop this checkZone() method? I see that it was used in the convertMGRSToGeodetic() method for error-checking. I suppose the breakMGRSString() method provides similar error-checking.
|
Could you please make the same changes in my Android branch just to save time? You are in context of issue now. |
|
I'll try to take a look at in the next few days. It's been a long time
since I made those changes.
Just for reference, NGA has Geotrans software for this conversion -
https://earth-info.nga.mil/GandG/geotrans/
as does GeoGraphicLib - https://geographiclib.sourceforge.io
…On 8/11/2019 2:15 PM, Wiehann Matthysen wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/gov/nasa/worldwind/geom/coords/MGRSCoordConverter.java
<#68 (comment)>:
> j = i;
- while (i < MGRSString.length() && Character.isDigit(MGRSString.charAt(i)))
+ while (i < MGRSString.length() && Character.isDigit(' '))
@gbburkhardt <https://github.com/gbburkhardt>, I'm not sure this
change is correct. The |Character.isDigit(' ')| call will always
return false.
------------------------------------------------------------------------
In src/gov/nasa/worldwind/geom/coords/MGRSCoordConverter.java
<#68 (comment)>:
> @@ -394,40 +397,6 @@ private MGRSComponents breakMGRSString(String MGRSString)
return null;
}
- /**
- * The function Check_Zone receives an MGRS coordinate string. If a zone is given, MGRS_NO_ERROR is returned.
- * Otherwise, MGRS_NOZONE_WARNING. is returned.
- *
- * @param MGRSString the MGRS coordinate string.
- *
- * @return the error code.
- */
- private long checkZone(String MGRSString)
@gbburkhardt <https://github.com/gbburkhardt>, is it ok for us to drop
this |checkZone()| method? I see that it was used in the
|convertMGRSToGeodetic()| method for error-checking. I suppose the
|breakMGRSString()| method provides similar error-checking.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#68?email_source=notifications&email_token=ABI6HFF5TSFB5I56EYK7MMDQEBJMPA5CNFSM4IKEUPY2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA5ER7A#pullrequestreview-272255228>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI6HFEGHNKOWLYUSI5VSLTQEBJMPANCNFSM4IKEUPYQ>.
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
|
|
It is a curious bit of code. I wonder if there wasn't a typo. How did
you catch it? Compiler warning?
…On 8/11/2019 3:15 PM, Wiehann Matthysen wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/gov/nasa/worldwind/geom/coords/MGRSCoordConverter.java
<#68 (comment)>:
> j = i;
- while (i < MGRSString.length() && Character.isDigit(MGRSString.charAt(i)))
+ while (i < MGRSString.length() && Character.isDigit(' '))
I think we can drop this while loop from the look of it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#68?email_source=notifications&email_token=ABI6HFH4GRLYQAYCOE5WFSLQEBQL7A5CNFSM4IKEUPY2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBGORDQ#discussion_r312751017>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI6HFHVUMD6Q6YURA7ECLDQEBQL7ANCNFSM4IKEUPYQ>.
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
|
|
@gbburkhardt, nope... there was no compiler warning. I was lucky to spot it in the diffs of the commit. |
|
You caught a bug. I checked what we have in production, and created another pull request . I added a unit test. Thanks. |
|
I'm closing this as a duplicate of PR #72. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
Pulled in changes from upstream PR #157 of @gbburkhardt.
Why Should This Be In Core?
Bugfixes to MGRS coordinate conversion is critical to have in core.
Benefits
More stable MGRS coordinate conversion logic.
Potential Drawbacks
None
Applicable Issues
Issue #67