trying out shifting coordinates by osana · Pull Request #903 · mapbox/mapbox-java

osana · 2018-10-11T02:29:05Z

No description provided.

langsmith

LGTM as a start. I understand what's going on too 😄

zugaldia · 2018-10-11T19:55:25Z

I like how this approach helps decouple the logic downstream. Capturing chat, let's include performance tests to understand any hits coming from the approach in this PR (introducing a default operation in all cases) vs only applying the shifting conditionally vs no changes.

lloydsheng · 2018-10-12T06:04:31Z

+
+public interface CoordinateShifter {
+
+  double shiftLat(double lat);


I'd suggest the API can be Coordinate shift(Coordinate coordinate), case the latitude and longitude used to calculate together.

@lloydsheng Thank you for pointing this out. This will be a pain because we are trying to do this in the constructor.

osana · 2018-10-12T18:34:20Z

@tobrun @LukasPaczos Could you have a look
I still need to work on toJson/fromJson adjustments.

langsmith · 2018-10-16T08:44:02Z

Update after chatting with @tobrun about this:

Getting access to @osana’s work in China plugin
- Merge @osana’s changes to master
- In the China Plugin project, I will use 6.6.0 of the Maps SDK and use exclude group as seen in the code example in https://stackoverflow.com/a/36103326/6358488 . I will use exclude group to block usage of the Maps SDK’s regular Java SDK version. I will also add a new dependency line that adds the Java SDK SNAPSHOT, which should include @osana’s shifting work by then.
Regarding testing:
- Timestamp comparison via simple logcat logging (@tobrun mentioned there might be some sort of framework for this)
- Profiling memory through Android Studio. GC for use in Android Studio. Run tests and see how much memory consumed. Compare to baseline non-shifting.
- Try to see if there's a way to test CPU usage
- Seeing the performance of C++ JNI bridge for shifting will be interesting

langsmith · 2018-10-17T01:59:51Z

+public interface CoordinateShifter {
+
+  /**
+   * Shift coordinagte values according to its algorithm.


Suggested change

* Shift coordinagte values according to its algorithm.

* Shift coordinate values according to its algorithm.

langsmith · 2018-10-17T02:00:17Z

+  List<Double> shiftLonLat(double lon, double lat);
+
+  /**
+   * Shift coordinagte values according to its algorithm.


Suggested change

* Shift coordinagte values according to its algorithm.

* Shift coordinate values according to its algorithm.

langsmith · 2018-10-17T02:00:30Z

+  List<Double> shiftLonLatAlt(double lon, double lat, double altitude);
+
+  /**
+   * Unshift coordinagte values according to its algorithm.


Suggested change

* Unshift coordinagte values according to its algorithm.

* Unshift coordinate values according to its algorithm.

tobrun · 2018-10-17T16:02:16Z

    try {
      // Use the current context to deserialize it
-      Type classType = Class.forName("com.mapbox.geojson." + geometryType);
+      Type classType = Class.forName("com.mapbox.geojson.AutoValue_" + geometryType);


is this change required for this PR specifically?

tobrun · 2018-10-17T16:02:37Z

-    } else {
-      rawCoordinates = json.getAsJsonArray();
-    }
+    rawCoordinates = json.getAsJsonArray();


is this change required for this PR specifically?

langsmith · 2018-10-22T03:34:52Z

@osana has expressed that she's comfortable with the state of this pr and that I can go ahead and merge to master so that next steps can happen

langsmith · 2018-10-22T06:39:56Z

@osana expressed her comfort with my merging this, so I'm merging this pr. This will allow for next steps as outlined in my comment above.

osana requested review from LukasPaczos, langsmith and tobrun October 11, 2018 02:29

langsmith approved these changes Oct 11, 2018

View reviewed changes

lloydsheng reviewed Oct 12, 2018

View reviewed changes

osana force-pushed the osana-cn-shifting branch 2 times, most recently from 54533c9 to c482d8b Compare October 12, 2018 18:14

osana force-pushed the osana-cn-shifting branch 4 times, most recently from 823e506 to 58ad1fb Compare October 16, 2018 22:52

langsmith reviewed Oct 17, 2018

View reviewed changes

osana force-pushed the osana-cn-shifting branch 2 times, most recently from e9b2123 to ccdffd2 Compare October 17, 2018 15:07

tobrun reviewed Oct 17, 2018

View reviewed changes

First attempt at Shifter implementation

6ae4c7c

osana force-pushed the osana-cn-shifting branch from ccdffd2 to 6ae4c7c Compare October 17, 2018 16:22

langsmith merged commit 5b6d18f into master Oct 22, 2018

langsmith deleted the osana-cn-shifting branch October 22, 2018 07:04


		public interface CoordinateShifter {

		double shiftLat(double lat);

	* Shift coordinagte values according to its algorithm.
	* Shift coordinate values according to its algorithm.

	* Unshift coordinagte values according to its algorithm.
	* Unshift coordinate values according to its algorithm.

Uh oh!

Conversation

osana commented Oct 11, 2018

Uh oh!

langsmith left a comment

Choose a reason for hiding this comment

Uh oh!

zugaldia commented Oct 11, 2018

Uh oh!

lloydsheng Oct 12, 2018 • edited by osana Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osana Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

osana commented Oct 12, 2018

Uh oh!

langsmith commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

langsmith Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

langsmith Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

langsmith Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

tobrun Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

tobrun Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

langsmith commented Oct 22, 2018

Uh oh!

langsmith commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lloydsheng Oct 12, 2018 •

edited by osana

Loading

langsmith commented Oct 16, 2018 •

edited

Loading