Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Bugs in Transform? #1574

Closed
ljbade opened this issue May 17, 2015 · 6 comments
Closed

Bugs in Transform? #1574

ljbade opened this issue May 17, 2015 · 6 comments
Labels

Comments

@ljbade
Copy link
Contributor

ljbade commented May 17, 2015

After poking around trying to figure out #1468 I have come to the conclusion that parts of the code in Transform that iOS has stopped excercising for a while has become stale.

There are some really strange bugs I have found particularly with Map::setZoom and Map::setLatLng which only Android has been using recently.

Looking at iOS it uses Map::setLatLngZoom in combination with Map::getZoom and Map::getLatLng to implement what used to be done with Map::setZoom and Map::setLatLng. Also none of the C++ test cases seem to use either of the those functions.

I can change Android to switch to the same code that iOS uses, but that will only cover up the bugs in the C++ layer. I might do this as a stop gap to get a working Android app until the bugs in the C++ side have been fixed.

Solutions I can think of:

  • remove Map::setZoom and Map::setLatLng since no one using it anymore
  • fix the functions and create test cases that cover all features of Transform to stop future bugs
  • leave Map::setZoom and Map::setLatLng alone and just leave Android and iOS using Map::setLatLngZoom

/cc @incanus @bleege @jfirebaugh

@bleege
Copy link
Contributor

bleege commented May 17, 2015

/cc @kkaefer

@jfirebaugh
Copy link
Contributor

If there's a bug in setZoom or setLatLng we should certainly fix it (and add tests). I'm hesitant to jump straight to that conclusion though. Have you been able to trace through and see where things go wrong?

@incanus
Copy link
Contributor

incanus commented May 18, 2015

Does order of calling matter? e.g. set center, but then set zoom and it loses/invalidates the center?

@ljbade
Copy link
Contributor Author

ljbade commented May 18, 2015

@incanus I was thinking along those lines, but I changed to using only setLanLngZoom and it workse slightly better but it is still rather broken.

@jfirebaugh
Copy link
Contributor

We determined this was due to problematic floating-point compiler options.

@bleege
Copy link
Contributor

bleege commented May 28, 2015

We determined this was due to problematic floating-point compiler options.

Awesome! So this means that compiler options have now been adjusted and things work on ARM again?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants