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

Accurate hit detection #710

Merged
merged 6 commits into from May 16, 2013
Merged

Accurate hit detection #710

merged 6 commits into from May 16, 2013

Conversation

ahocevar
Copy link
Member

With this change, hit detection for lines and points gets very
accurate, because the vector renderer instance keeps track of
line widths and point symbol sizes. After doing a bbox query in
the RTree, returned lines and points are evaluated against the
thresholds of their line width or symbol size. The KML example
with its different symbolizers now has getFeatureInfo too to
show this in action.

With this change, hit detection for lines and points gets very
accurate, because the vector renderer instance keeps track of
line widths and point symbol sizes. After doing a bbox query in
the RTree, returned lines and points are evaluated against the
thresholds of their line width or symbol size. The KML example
with its different symbolizers now has getFeatureInfo too to
show this in action.
@ahocevar
Copy link
Member Author

This needs a bit more work to ensure accurate results also after resolution changes.

The reason for this change is that symbolSizes and maxSymbolSize
on the instance will be wrong as soon as the resolution changes
and cached tiles are used. It turned out that the approach used
now has several advantages: smaller symbolSizes objects, no need
to merge symbolSizes objects, and cache management for free (no
risk of memory leaks). Note that the symbolSizes and
maxSymbolSize for each tile are not strictly tile specific -
they represent the rendering pass that created the tile. This
has no negative side effects, and it has the advantage that
there is not a single additional loop needed to create these
structures.
@ahocevar
Copy link
Member Author

Now this is ready for review.

var cachedTile = this.tileCache_.get(key);
var symbolSizes = cachedTile[1];
var maxSymbolSize = cachedTile[2];
var hw = maxSymbolSize[0] / 2;
Copy link
Member

Choose a reason for hiding this comment

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

hw = half width? Maybe a bit more verbose variable name? But not really important.

[[location[0] - halfWidth, location[1] - halfHeight],
[location[0] + halfWidth, location[1] + halfHeight]]);
coordinates = geom.getCoordinates();
if (ol.extent.containsCoordinate(symbolBounds, coordinates)) {
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering why you chose to change the function itself, and not just call it in a for loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I had before. But it got a bit ugly doing another check for geometry type (POINT vs. MULTIPOINT) and do two different things.

POINT: getCoordinates returns [x, y]
MULTIPOINT: getCoordinates returns [[x1, y1], [x2, y2]]

Any other suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

But you can do the same thing as what you're doing now in the function or not?

coordinates = goog.isArray(coordinates[0]) ? coordinates : [coordinates];
for (var i=0, ii=coordinates.length; i<ii; ++i) {
  if (ol.extent.containsCoordinate(symbolBounds, coordinates[i])) {
    result.push(candidate);
    break;
  }
}

Personally I'm in favour of not having this in the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will change.

currentSize = goog.isDef(this.symbolSizes_[id]) ?
this.symbolSizes_[id] : [0];
currentSize[0] = Math.max(currentSize[0], context.lineWidth);
this.symbolSizes_[id] = currentSize;
Copy link
Member

Choose a reason for hiding this comment

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

so why is currentSize an array with only one entry? I see in the code block below it's an array of 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid mixed types (for points, it is an array with 2 entries). But I can change this to mixed type if this would look better to you.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation, no I think this is fine.

@bartvde
Copy link
Member

bartvde commented May 16, 2013

LGTM now @ahocevar please merge after Travis says it's okay

ahocevar added a commit that referenced this pull request May 16, 2013
@ahocevar ahocevar merged commit 7ed51f9 into openlayers:master May 16, 2013
@ahocevar ahocevar deleted the hit-detection branch May 16, 2013 09:10
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.

None yet

2 participants