[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate #6075

mike-marcacci · 2018-01-28T21:59:05Z

NOTE: this bug cannot be reproduced in mapbox-gl-js 0.44.0, because it is blocked by this other, more serious bug.

Using v0.43.0, map.queryRenderedFeatures() doesn't account for the icon-rotate layout property of symbol layers, and returns runs as if it were set to 0.

VIEW SCREENCAST

The text was updated successfully, but these errors were encountered:

Closes #6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.

* Add failing test for issue #6075 * Fix incorrect textPixelRatio argument Closes #6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.

* Add failing test for issue mapbox#6075 * Fix incorrect textPixelRatio argument Closes mapbox#6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.

anandthakker · 2018-02-14T17:05:03Z

The actual issue here was due to icon-offset -- see #6135 (comment)

mike-marcacci · 2018-02-20T04:37:48Z

Hi @anandthakker, thanks for working on this. Unfortunately, this isn't fixed in 0.44.1. I've made a fiddle that shows the issue: https://jsfiddle.net/xzdsxdd9/22/

Note that the cursor should be set to "pointer" when hovered over the wrench, and "" otherwise. The wrench is rotated 90 degrees, but the "hover" zone doesn't consider the rotation.

anandthakker · 2018-02-20T13:00:09Z

Oof, thanks @mike-marcacci. Reopening for the icon-rotate case.

anandthakker · 2018-03-08T20:09:46Z

Per chat with @ChrisLoer, the best way to fix this is most likely to use a covering of circles (like we do for line-placed labels) rather than a box.

Refs #6075

santinogue · 2018-03-14T15:50:56Z

@anandthakker @mike-marcacci is there a workaround for this?

mike-marcacci · 2018-04-12T20:03:08Z

@santinogue there isn't one that I've found. Fortunately, while this bug is live for us, we're in a "closed beta" of sorts, so we've been able to explain how to "click above the handle" to the limited users... but it's definitely a bad experience :-/

Depending on exactly what you're trying to accomplish, you might consider arranging a set of small transparent icons where you would expect the visual area to be, and use them as click/mouse targets...

mike-marcacci · 2018-04-30T18:32:25Z

Hey @anandthakker! I just wanted to check up on this – is this guy planned for an upcoming release by chance? If not, I'd be happy to work on it if you could elaborate on your "covering of circles" strategy.

anandthakker · 2018-05-03T18:00:36Z

Hey @mike-marcacci - nope, this isn't specifically planned for a release yet. The strategy @ChrisLoer and I had in mind was to use "collision circles" to represent rotated symbol features, rather than viewport-aligned rectangles.

Currently, we only use collision circles for text labels that have symbol-placement: line. This happens in CollisionFeature:

mapbox-gl-js/src/symbol/collision_feature.js

Line 75 in 66c0e33

_addLineCollisionCircles(collisionBoxArray: CollisionBoxArray,

, via this call to addSymbol in symbol_layout.js:

mapbox-gl-js/src/symbol/symbol_layout.js

Lines 207 to 211 in 66c0e33

    
           bucket.symbolInstances.push(addSymbol(bucket, anchor, line, shapedTextOrientations, shapedIcon, bucket.layers[0], 
        
               bucket.collisionBoxArray, feature.index, feature.sourceLayerIndex, bucket.index, 
        
               textBoxScale, textPadding, textAlongLine, textOffset, 
        
               iconBoxScale, iconPadding, iconAlongLine, iconOffset, 
        
               feature, glyphPositionMap, sizes));

So we'd need to refactor these a bit to add an additional path for using a set of collision circles also in the case where we have symbol-placement: point and the symbol is not oriented orthogonal with respect to the viewport.

Also worth noting #5474, which would simplify some of the collision circle logic.

ChrisLoer · 2018-05-03T21:33:45Z

Agree with @anandthakker that the full fix here is to use collision circles to represent rotated icon geometry -- but just wanted to note that part of the problem may be easier to solve:

When icon-rotate is combined with icon-offset, the center of the icon's collision box gets placed in the wrong place. For a lot of (relatively square) icons, the center position may be the really important part. Fixing this should be easier because it doesn't require us to change the way we represent collision geometry.
icon-rotate changes the shape of the icon's, ie putting a rectangle at an X degree angle relative to the viewport. This is the harder change. It's definitely something we want to support, but it's probably more important for icons that have a wide aspect ratio (so that the "shape change" is dramatic). Just spitballing: as a placeholder fix that doesn't require a new type of geometry, we could be conservative and do collision based on the smallest viewport-aligned rectangle that contains the rotated rectangle.

ChrisLoer · 2018-05-05T02:27:13Z

I experimented with a simple implementation of this in collision_feature.js, and at least for point placement it works well (the bus icon and the "GeoJSON Source 1" label both have the same anchor, with different offsets and rotations). The "envelope" strategy seems like it might be fine -- 45 degrees is a worst case in terms of being "too conservative", but visually it doesn't seem like the box is wildly big.

I'm going to look into doing the harder part of #4798 (handling line labels) to see if it makes sense to pull into one PR. I'll also take a look at #4861 (handling rotation-alignment: map) to see if the envelope strategy is promising there.

hyperknot · 2018-05-06T10:13:46Z

@ChrisLoer, what's the recommended way to use collision circles with queryRenderedFeatures? My idea is to simply use 'circle-opacity': 0, on a fake circle layer, in case I can cover my symbols with a single circle.

How would you cover symbols with multiple circles? Would you add multiple layers with 0 opacity?

I'm trying to find a workaround for #6621

ChrisLoer · 2018-05-07T18:28:35Z

Hi @hyperknot sorry for the confusion, but "collision circles" here refers to an underlying implementation detail -- there's nothing that's directly accessible to you as an SDK user. The good news is, I think #6621 is mainly a duplicate of a bug that we recently fixed and should ship this week! 😄

Fixes issue #6075. Does not fix collision detection for point symbols with *-rotation-alignment: map, or for line-placed symbols with *-offset set.

ChrisLoer · 2018-05-09T20:31:12Z

Fixed *-rotate for point-placed symbols in #6631 (didn't make the .45 release, so it'll show up in .46).

@mike-marcacci and @santinogue, will this address your current issues? We're still tracking offsets for line-placed labels separately as #4798 and point labels with rotation-alignment: map in #4861.

mike-marcacci · 2018-05-14T22:10:55Z

Hi @ChrisLoer! Thank you so much for working on this. I just built master (making sure it includes #6631) and tested it against the example in the fiddle above, and sadly it didn't work. I've got a little bit of time right now to look into this further, so I'll post back once I have more to offer!

mike-marcacci · 2018-05-14T22:19:52Z

Actually, scratch that. It looks like it built from a stale cache. I can confirm that this is fixed!! Thanks SO much!

mike-marcacci mentioned this issue Jan 28, 2018

[regression] 0.44.0 map.queryRenderedFeatures() completely ignores symbols #6074

Closed

anandthakker self-assigned this Feb 12, 2018

anandthakker pushed a commit that referenced this issue Feb 12, 2018

Add failing test for issue #6075

a0926cb

anandthakker mentioned this issue Feb 12, 2018

Fix incorrect textPixelRatio argument #6135

Merged

anandthakker closed this as completed in #6135 Feb 13, 2018

anandthakker changed the title ~~[bug] map.queryRenderedFeatures() doesn't account for icon-rotate~~ [bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset Feb 14, 2018

anandthakker mentioned this issue Feb 14, 2018

Unable to click/hover a text element with an offset #6146

Closed

anandthakker reopened this Feb 20, 2018

anandthakker changed the title ~~[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset~~ [bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate Feb 20, 2018

anandthakker added the bug 🐞 label Feb 20, 2018

anandthakker added this to the 0.45.0 milestone Mar 8, 2018

anandthakker pushed a commit that referenced this issue Mar 13, 2018

Update regression test to account for icon-rotate

21a6b37

Refs #6075

anandthakker mentioned this issue Mar 21, 2018

Listeners do not take into account text-rotate layout property #6373

Closed

anandthakker removed this from the 0.45.0 milestone Mar 23, 2018

ChrisLoer mentioned this issue May 5, 2018

Respect text-offset in line-placed symbol collision handling #4798

Open

ChrisLoer mentioned this issue May 7, 2018

Collision boxes don't account for symbol-placement:point/rotation-alignment: map #4861

Open

ChrisLoer added a commit that referenced this issue May 7, 2018

Handle *-rotate for point-placed symbols.

5b6ea5c

Fixes issue #6075. Does not fix collision detection for point symbols with *-rotation-alignment: map, or for line-placed symbols with *-offset set.

ChrisLoer mentioned this issue May 7, 2018

Handle *-rotate for point-placed symbols. #6631

Merged

4 tasks

ChrisLoer added a commit that referenced this issue May 7, 2018

Handle *-rotate for point-placed symbols.

728b214

Fixes issue #6075. Does not fix collision detection for point symbols with *-rotation-alignment: map, or for line-placed symbols with *-offset set.

jfirebaugh mentioned this issue May 8, 2018

If you set layout.icon-offset in a geojson layer, mouse events do not find the icon #6624

Closed

ChrisLoer added a commit that referenced this issue May 9, 2018

Handle *-rotate for point-placed symbols.

e29bb8b

Fixes issue #6075. Does not fix collision detection for point symbols with *-rotation-alignment: map, or for line-placed symbols with *-offset set.

ChrisLoer closed this as completed May 9, 2018

elementalhub mentioned this issue Mar 12, 2020

Mouse events do not trigger correctly on LineStrings that are offset with line-offset #9419

Open

chippieTV mentioned this issue Sep 4, 2021

Latest main (Typescript) has some regressions in Firefox maplibre/maplibre-gl-js#305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate #6075

[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate #6075

mike-marcacci commented Jan 28, 2018 •

edited

Loading

anandthakker commented Feb 14, 2018

mike-marcacci commented Feb 20, 2018 •

edited

Loading

anandthakker commented Feb 20, 2018

anandthakker commented Mar 8, 2018

santinogue commented Mar 14, 2018

mike-marcacci commented Apr 12, 2018

mike-marcacci commented Apr 30, 2018

anandthakker commented May 3, 2018

ChrisLoer commented May 3, 2018

ChrisLoer commented May 5, 2018

hyperknot commented May 6, 2018 •

edited

Loading

ChrisLoer commented May 7, 2018

ChrisLoer commented May 9, 2018

mike-marcacci commented May 14, 2018 •

edited

Loading

mike-marcacci commented May 14, 2018

[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate #6075

[bug] map.queryRenderedFeatures() doesn't account for {text,icon}-offset or {text,icon}-rotate #6075

Comments

mike-marcacci commented Jan 28, 2018 • edited Loading

anandthakker commented Feb 14, 2018

mike-marcacci commented Feb 20, 2018 • edited Loading

anandthakker commented Feb 20, 2018

anandthakker commented Mar 8, 2018

santinogue commented Mar 14, 2018

mike-marcacci commented Apr 12, 2018

mike-marcacci commented Apr 30, 2018

anandthakker commented May 3, 2018

ChrisLoer commented May 3, 2018

ChrisLoer commented May 5, 2018

hyperknot commented May 6, 2018 • edited Loading

ChrisLoer commented May 7, 2018

ChrisLoer commented May 9, 2018

mike-marcacci commented May 14, 2018 • edited Loading

mike-marcacci commented May 14, 2018

mike-marcacci commented Jan 28, 2018 •

edited

Loading

mike-marcacci commented Feb 20, 2018 •

edited

Loading

hyperknot commented May 6, 2018 •

edited

Loading

mike-marcacci commented May 14, 2018 •

edited

Loading