Skip to content

MGRS Conversion fix #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 14, 2019
Merged

MGRS Conversion fix #72

merged 6 commits into from
Aug 14, 2019

Conversation

gbburkhardt
Copy link
Member

@gbburkhardt gbburkhardt commented Aug 14, 2019

Description of the Change

Fix two problems:

a) Index out of b typo in convertMGRSToUPS; clearly
the index should have been 1 instead of 12.ounds error due to
b) Conversions from geodetic to MGRS would fail for low southern
latitudes (zone 0). Geotrans 3.7 has this problem fixed. The WorldWind
conversion code was derived from NGA's Geotrans. Test case:
Lat: -89.345400 deg, Lon: -48.930600 deg ==> MGRS: AZN 45208 47747

Why Should This Be In Core?

Correct conversion from geodetic to MGRS

Benefits

Potential Drawbacks

None

Applicable Issues

Issue #67.

a) Index out of bounds error due to typo in convertMGRSToUPS; clearly
the index should have been 1 instead of 12.
b) Conversions from geodetic to MGRS would fail for low southern
latitudes (zone 0). Geotrans 3.7 has this problem fixed. The WorldWind
conversion code was derived from NGA's Geotrans. Test case:
Lat: -89.345400 deg, Lon: -48.930600 deg ==> MGRS: AZN 45208 47747
@wcmatthysen wcmatthysen added the bug Something isn't working label Aug 14, 2019
@wcmatthysen wcmatthysen added this to the WWJ-CE 2.2.0 milestone Aug 14, 2019
@wcmatthysen wcmatthysen mentioned this pull request Aug 14, 2019
Made some formatting fixes as well as fixing a spelling mistake in one
of the comments.
Moved the UTM_MGRS_test class to the correct location. It should be in
the test folder so that it gets executed as part of the unit tests.
- Split up some of the methods into smaller self-contained methods.
- Renamed some of the variables for clarity.
- Changed the formatting by replacing tabs with spaces. Made some other
  formatting changes so that the code fits in with rest of code-base.
- Added license-header at the top.
@wcmatthysen
Copy link
Member

wcmatthysen commented Aug 14, 2019

@gbburkhardt, I made some small fixes. The UTM_MGRS_test class should have been in the test folder where Gradle can pick it up during the unit-test phase. I also renamed it to CoordTest as we can add tests for all coordinate conversions there in the future. The builds failed because JUnit is only a test-dependency and if you add a test class to the src folder (instead of the test folder) it won't pick up the JUnit classes when compiling. If you want to see why a build fails you can click the red-cross that appears next to the commit and go to the details link where it will show you the different Travis builds. You can then select one of them to see the logs to determine the reason for the build-failure.

@EMaksymenko
Copy link
Member

Hello, guys! Could you please do not forgot to make the same changes in my Android port when you finished. I do not want to lost convertor code-base synchronization between Java and Android. It will be harder for me to dive deep in to current issue context then you. Thanks.
WorldWindEarth/WorldWindAndroid#16

@wcmatthysen
Copy link
Member

@Sufaev, I can help with the port of these fixes to the Android code-base.

@EMaksymenko
Copy link
Member

@wcmatthysen I have unreleased PR 16 in Android repository, which contain ported coordinate converters and graticules from Java. It will be perfect to add commit with all changes related to latest MGRS fixes to the same PR.

By the way, I do not hear @emxsys for a long time. That's why PRs are still unreleased. Is he ok?

@wcmatthysen
Copy link
Member

@Sufaev, I'll have a look at that pull-request once we are done merging this one.

I don't know. I haven't heard from him in a while.

@gbburkhardt
Copy link
Member Author

Well, as you can see, I still don't like the verbose coding style w.r.t. curly brackets. 😉
But I see the merit in keeping the style in the code base uniform. I'm fine with the changes you made. Merge when ready, as far as I'm concerned.

@gbburkhardt
Copy link
Member Author

@wcmatthysen : I notice that you added a NASA copyright to the MGRS unit test file. Is that appropriate? NASA didn't write that code. Can a copyright be assigned to an entity without its knowledge or permission? I'm not worried, just curious.

@wcmatthysen
Copy link
Member

@gbburkhardt, I also don't like the curly braces on a separate line when it comes to styling. I think it is just important to keep the coding-style consistent with respect to the rest of the code-base. It might be something that we can discus in future depending on what happens to the upstream repository. If they suddenly start to contribute changes again then we might have to stick with the coding-style as it will be easier to pull in changes from them.

I think we need to add the copyright at the top of each new file as the entire code-base is licensed under the NASA open-source license agreement. It doesn't matter if a NASA developer wrote the code or not. As contributors we agree to adhere to the license when we make contributions.

@wcmatthysen wcmatthysen merged commit 97da99e into WorldWindEarth:develop Aug 14, 2019
@wcmatthysen wcmatthysen mentioned this pull request Aug 14, 2019
@gbburkhardt
Copy link
Member Author

gbburkhardt commented Aug 14, 2019 via email

@wcmatthysen
Copy link
Member

@gbburkhardt, no worries. I am more worried about changing the coding-style and then the upstream repository suddenly becomes active again (though that looks doubtful at this point). If the upstream developers add changes and we need to pull those changes through to our repository here then we'll have a hard-time sorting through the changes if we have a lot of changes on our side related to code formatting. So, at this point, keeping the coding style as-is is more a case of us keeping as much in sync with the upstream repository as possible. Maybe down the line if we see that the upstream repository is truly dead we can consider switching to a more compact coding style. We'll have to wait and see what happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants