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

GeoJSON point clustering #5724

Merged
merged 13 commits into from
Jul 27, 2016
Merged

GeoJSON point clustering #5724

merged 13 commits into from
Jul 27, 2016

Conversation

mourner
Copy link
Member

@mourner mourner commented Jul 19, 2016

Closes #320, a work in progress. cc @jfirebaugh

  • add Supercluster type to urlOrGeoJSON variant
  • instantiate Supercluster index in GeoJSONSource::Impl setGeoJSON if cluster is true in options and use it in createTile
  • see if we can avoid adding geojson to include paths and public headers
  • bump supercluster.hpp and kdbush.hpp to fix compiler warnings in iOS
  • fix cluster wrapping near the date line
  • make sure all platforms are building
  • clean up the code
  • un-ignore clustering test in render suite and make sure it passes
  • review by @jfirebaugh
  • rebase and add [core] prefix to commits
  • rejoice!

@mourner mourner added the Core The cross-platform C++ core, aka mbgl label Jul 19, 2016
@mention-bot
Copy link

@mourner, thanks for your PR! By analyzing the annotation information on this pull request, we identified @jfirebaugh, @yhahn and @boundsj to be potential reviewers

@mourner
Copy link
Member Author

mourner commented Jul 19, 2016

OK, clustering now seems to be working!

@1ec5 1ec5 added the feature label Jul 19, 2016

namespace mbgl {

class GeoJSON {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this header file and keep using the GeoJSON symbol, but change the definition to:

namespace mbgl {

using GeoJSON = mapbox::geojson::geojson;

} // namespace mbgl

This follows a convention established by mbgl::Geometry, mbgl::Feature, etc. It keeps the naming conventions within the project more consistent, and allows for definition changes (like this one) to happen with minimal changes to downstream code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do.

@mourner
Copy link
Member Author

mourner commented Jul 27, 2016

The tests passed, woohoo! Now what's left is rebase + cleanup + merge.

@@ -1705,6 +1705,7 @@
"$(OTHER_CFLAGS)",
"$(variant_cflags)",
"$(geometry_cflags)",
"$(geojson_cflags)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be corresponding changes in macos.xcodeproj?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe — let me add them. I wonder why it's not caught by the builds though.

@mourner mourner merged commit 9ecc0d9 into master Jul 27, 2016
@mourner mourner deleted the supercluster branch July 27, 2016 17:18
@mourner mourner mentioned this pull request Jul 27, 2016
@leekuo
Copy link

leekuo commented Jul 31, 2016

Any idea when this will make it into the iOS SDK release?

@1ec5
Copy link
Contributor

1ec5 commented Aug 11, 2016

@leekuo, this functionality is now in ios-v3.4.0-alpha.1. Please note the caveats in #320 (comment).

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 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

point clustering
5 participants