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

[core] Fix Integer overflow when converting tileCoordinates to LatLon #15560

Merged
merged 3 commits into from
Sep 5, 2019

Conversation

zmiao
Copy link
Contributor

@zmiao zmiao commented Sep 4, 2019

Fix an issue that potential integer overflow would happen with high zoom level.

@zmiao zmiao self-assigned this Sep 4, 2019
@zmiao zmiao requested a review from 1ec5 as a code owner September 4, 2019 14:05
@zmiao zmiao requested a review from a team September 4, 2019 14:05
@zmiao zmiao merged commit f53143f into master Sep 5, 2019
@zmiao zmiao deleted the zmiao-fix-geometry-conversion branch September 5, 2019 08:08
@friedbunny friedbunny added this to the release-ristretto milestone Sep 5, 2019
@friedbunny friedbunny added the Core The cross-platform C++ core, aka mbgl label Sep 5, 2019
@chloekraw
Copy link
Contributor

@zmiao in addition to queryRenderedFeatures returning incorrect results at high zoom levels, what other operations were negatively affected by this bug?

@zmiao
Copy link
Contributor Author

zmiao commented Sep 6, 2019

@chloekraw it will also affect querySourceFeatures.

@chloekraw
Copy link
Contributor

Thanks @zmiao. Thank you for remembering to provide a changelog entry! It’s much appreciated. Your changelog entry is accurate and concise, two things that are hard to get right about changelog entries. Great job.

Our changelog entries should target someone like me as the audience. A product manager / CEO should be able to understand what the bug that’s fixed is and what the new features added do. Why? So that they will want to upgrade to the release! And if someone like me understands the changelog, then certainly developers who use the mobile SDK will as well, so we’d know we’re serving our main audience well.

The reality is this is very hard to do well, and I understand that. Many of our changelog entries could use improvement from this perspective. Forgive me for picking on your changelog entry as a demonstration, but I’m taking this opportunity to clarify what changelog entries should be because I happen to know how this bug manifests.

Fixed an issue of integer overflow when converting tileCoordinates to LatLon.

This doesn’t mean much to me, and I’d wager it wouldn’t mean much to most casual users of our mobile SDKs either, as we don’t know when converting tileCoordinates to LatLon happens. To show our audience what the impact of fixing this bug is, you could write:

Fixed a bug that caused queryRenderedFeatures and querySourceFeatures to return incorrect coordinates at zoom levels 20 and higher.

Developers know if they are using queryRenderedFeatures or querySourceFeatures and need to upgrade to this release. So this would be a good changelog entry.

Let’s say this integer overflow bug causes many different bugs and you’re not sure if you know of all of them. You could then keep what you wrote but include an example to put the problem in context for the developer. For example:

Fixed an integer overflow bug that caused issues such as queryRenderedFeatures and querySourceFeatures returning incorrect coordinates at zoom levels 20 and higher.

I hope this is clear and that it helps elucidate what we’re looking for from changelogs! Thanks as always for your fantastic contributions. I’m really happy to have you on the team.

cc @mapbox/gl-native @mapbox/maps-android @mapbox/maps-ios

@zmiao
Copy link
Contributor Author

zmiao commented Sep 9, 2019

@chloekraw Thanks a lot for bringing this up! It is quite a helpful and clear guidance that how I could write changelogs in the future.

zmiao added a commit that referenced this pull request Sep 9, 2019
jmkiley added a commit that referenced this pull request Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants