Skip to content
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

Render only visible items on the map #217

Merged
merged 9 commits into from
Apr 17, 2018

Conversation

zamesilyasa
Copy link
Contributor

Implemented rendering only visible items on the map(issue #82 )
Added VisibleClusteringDemoActivity activity which demonstrates clustering of 20k items.

Review on Reviewable

@barbeau
Copy link
Collaborator

barbeau commented Oct 19, 2015

@broady or @markmcd could you rerun this build on Travis? The initial build failed with com.android.ddmlib.ShellCommandUnresponsiveException, which isn't related to the PR contents.

@barbeau
Copy link
Collaborator

barbeau commented Oct 21, 2015

Thanks, looks like the Travis build passed now. I've been seeing occasional failures due to com.android.ddmlib.ShellCommandUnresponsiveException on Travis builds on other projects too. Increasing the timeout doesn't seem to help, so right now unfortunately the workaround is just to poke Travis again, and it usually passes on the second attempt.

@meierjan
Copy link

+1 for merge

@Dima564
Copy link

Dima564 commented Jan 7, 2016

+1

@broady
Copy link
Contributor

broady commented Jan 11, 2016

Mark or @jfschmakeit can you look at this more closely?

@markmcd
Copy link
Contributor

markmcd commented Feb 15, 2016

Awesome - thanks for this contribution. It's looking great on the device. Just a couple of comments around the code.

Oh and I'll re-run the Travis builds when needed, they're very flaky at the moment.

cluster();
}

public void setClusterOnlyVisibleArea(boolean onlyVisibleArea) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that a user should have to provide an algorithm as well as specifying this property. Can you refactor such that this property is defined on the Algorithm implementation? e.g. add a method to Algorithm like boolean shouldReclusterOnMapMovement()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about back compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we could use Java 8 we could add a default method to the interface, but alas that's not an option.

How about extending Algorithm (e.g. ViewBasedAlgorithm) and use that to determine instead. You may even be able to move the onCameraChangeListener code in there, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this variant too, but in this case I will have to use instanceof to check, if this algorythm is ViewBasedAlgorithm, but if this algorithm will be decorated(like you do with PreCachingAlgorithmDecorator), it will not work at all. That's why I removed this decoration by default

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, good point. My main concern is that the behaviour of using setClusterOnlyVisibleArea on anything other than the new Algorithm you've introduced is undefined.

The only other option available is to break the Algorithm interface by adding the method suggested above. I've thought about this a bit and I think that it's the right thing to do but I (or whoever releases the next version to Maven) will need to make sure we bump the version number appropriately so it's clear what's happening.

What do you think about that? Reasonable? Or too drastic?

@barbeau
Copy link
Collaborator

barbeau commented Feb 15, 2016

FYI - looks like the Travis timeout issue should hopefully be fixed in 2.0.0-beta3 - see https://code.google.com/p/android/issues/detail?id=189764#c17 for details.

ScreenBasedAlgorithmAdapter is now derived from NonHierarchicalDistanceBasedAlgorithm
Added new ScreenBasedAlgorithm interface. Implementing this interface, Algorithm has map position and can skip clustering if it is not required.
Removed usage of instanceof checks in ClusterManager, which broke algorithms logic, in case of decorations.

googlemaps#82
@larten
Copy link

larten commented Mar 11, 2016

I think the getVisibleBounds() method in the VisibleNonHierarchicalDistanceBasedAlgorithm class not working correctly. On bigger screens the bounds not matching to the screen and the displayed area with markers is more bigger. I've tried to fix it with getVisibleRegion() method and calculation, but if user rotate the map, this solution is not working :)
Do you have any idea?

@zamesilyasa
Copy link
Contributor Author

What screen size/dimension do you use?
Also, if you rotate the map, you have to recluster everything.
There is updateViewSize() method in the latest update, call it with the new map size and recluster it.

@larten
Copy link

larten commented Mar 11, 2016

I've checked with a 480x800 and a 1920x1080 resolution.

Now, I've solved with this code:

Something wrong with code insertation :(

private Bounds getVisibleBounds(int zoom) {
if (mMapCenter == null) {
return new Bounds(0, 0, 0, 0);
}
Point p = PROJECTION.toPoint(mMapCenter);
Point left = PROJECTION.toPoint(this.left);
Point right = PROJECTION.toPoint(this.right);

    return new Bounds(Math.min(left.x, right.x), Math.max(left.x, right.x),
            Math.min(left.y, right.y), Math.max(left.y, right.y));
}

private LatLng left;
private LatLng right;

@Override
public void onCameraChange(CameraPosition cameraPosition) {
    VisibleRegion visibleRegion = map.getProjection().getVisibleRegion();
    mMapCenter = cameraPosition.target;
    left = visibleRegion.farLeft;
    right = visibleRegion.nearRight;
}

@zamesilyasa
Copy link
Contributor Author

I believe you use too old code. I don't use map bounds anymore. Please, pull the latest version of the patch

@larten
Copy link

larten commented Mar 11, 2016

Okey, thank you!

@meierjan
Copy link

Any idea when this is gonna be merged?

@lmbd
Copy link

lmbd commented Sep 1, 2016

Is it a part of the plan to integrate / merge this functionality?

@zamesilyasa
Copy link
Contributor Author

I am just trying to keep it up to date

@lfdversluis
Copy link

Is there any reason not to merge this? (besides perhaps squashing commits)

@zamesilyasa
Copy link
Contributor Author

Commits will be squashed by merge, isn't it?

@lfdversluis
Copy link

Someone with merge rights could, indeed.

jeffdgr8 and others added 2 commits December 21, 2016 01:45
If an Algorithm reference is already a ScreenBasedAlgorithm, then it can
be passed as is to setAlgorithm(ScreenBasedAlgorithm). Only if it is not
already a ScreenBasedAlgorithm is it then wrapped with a
ScreenBasedAlgorithmAdapter.
…m-wrap

Don't wrap Algorithm if already implements ScreenBasedAlgorithm
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@jeffdgr8
Copy link
Contributor

@googlebot I consent to have my commit merged to this project. Looking forward to having this pull request as well as my other two that are pending merged into the base project.

@b1n0m13
Copy link

b1n0m13 commented May 5, 2017

@zamesilyasa I noticed that NonHierarchicalViewBasedAlgorithm.shouldReclusterOnMapMovement() always return true, causing recluster on map movement even if it is just centering at selected cluster.
So when user taps at cluster to see info window it is shown only for a second, then disappear.

@zamesilyasa
Copy link
Contributor Author

zamesilyasa commented Jun 30, 2017

@b1n0m13
I had this problem before, but I solved it with custom popup windows. I had a view on top of the map, which showed popup windows. It was not hard to implement, because map is centered after click. So just show a popup at the center of a layout.

always return true

That's the core idea of the algorithm - recluster on map movement.

If you will disable this in case popup is shown and scroll the map, you will see an empty square of the map, what breaks consistency between visual representation and data set

@zamesilyasa
Copy link
Contributor Author

@markmcd
Is there any chance to merge it into dev?

@manish-bansal
Copy link

Will this be merged with main code?

@stephenmcd stephenmcd merged commit 42e8df3 into googlemaps:master Apr 17, 2018
@stephenmcd
Copy link
Contributor

This is merged now, thanks all.

@manish-bansal
Copy link

Will there be new release with these changes?

@manish-bansal
Copy link

Is there any plan to release this as part of next release? Managing gradle dependencies are much easier than moving the aar in multiple project.

@tmiyamon
Copy link

I also want to use this feature in my project with gradle dependencies. Please release it !

@lordplagus02
Copy link

+1 Would love this

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

Successfully merging this pull request may close these issues.