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

Vertices with attached symbol styles don't draw correctly in Mapbox V2 on iPhone. #1031

Closed
jaybo opened this issue Jan 21, 2021 · 4 comments
Closed

Comments

@jaybo
Copy link

jaybo commented Jan 21, 2021

mapbox-gl-js version: 2.0.1
mapbox-gl-draw version: 1.2.0

While there are numerous outstanding bugs regarding general Draw performance, this one seems unique in that repositioning vertices doesn't even start on an iPhone 6s until the touch point is lifted.

Steps to Trigger Behavior

  1. Create a geojson polyline.
  2. Use a style which includes a symbol on the active direct_select vertex (trash can in video below).
  3. Note serious lag on iPhone 6s.
  4. Desktop performance is OK, but still lots of flashing as points are selected.
  5. If the symbol is removed from the style, the performance is greatly improved, although there is still some drag delay.
  6. The issue also happens with geojson points which have attached symbols.

Here is the symbol definition:

// trash can ACTIVE for line vertices
{
    id: "gl-draw-polygon-and-line-vertex-trash-active",
    type: "symbol",
    filter: [
        "all",
        ["==", "meta", "vertex"],
        ["==", "$type", "Point"],
        ["!=", "mode", "static"],
        ["==", "active", "true"],
    ],
    layout: {
        "icon-image": "trash",
        "icon-allow-overlap": true,
        "icon-ignore-placement": true,
        "icon-size": 1.0,
        "icon-offset": [40, -40],
    },
},

Expected Behavior

Vertex dragging should be fluid even with symbols. Lacking this, having a hook so that I can manually disable the symbol while dragging would be an acceptable alternative.

Actual Behavior

It appears that drawing of the intermediate drag positions doesn't happen until the touch operation completes.

WIN_20210121_12_43_52_Pro.mp4
@jaybo
Copy link
Author

jaybo commented Feb 11, 2021

Just to prove this issue isn't due to my code, I've created a minimal subset.
Open the following link on an iPhone:

  1. Create a line
  2. Edit the line points and note extreme lagging and repaint bugs.

Demo:
https://bl.ocks.org/jaybo/d41c527e73450e7331ca8f03d055cc31

Source:
https://gist.github.com/jaybo/d41c527e73450e7331ca8f03d055cc31

@jaybo
Copy link
Author

jaybo commented Feb 12, 2021

You'll note that the initial drag completes correctly and rapidly, but then a queue of outdated drag events are drawn. Which would imply that someplace in draw push(json) is called needlessly on each move event, or the queue isn't being properly flushed at some later point, I'm guessing the lag has nothing to do with the symbol attached to the point, but this just creates enough overhead to exacerbate an underlying draw issue.

This issue is a showstopper for me.

@jaybo
Copy link
Author

jaybo commented Jun 29, 2021

Four months later and no comments, no workarounds, and the perf issues just keep growing with the number vertices.

I did some profiling and in one situation 50% of the time is spent in toGeoJSON() which is which in turn calls JSON.parse/JSON.stringify TWICE for each feature for each frame. Seems like this could be optimized by adding a "dirty" flag and only performing the conversion when the coordinates or properties actually change. Or at the very least, change line 39
of feature.js from:

coordinates: this.getCoordinates(),

to

coordinates: this.coordinates,

and avoid one cycle through JSON.parse / JSON.stringify.

@jaybo
Copy link
Author

jaybo commented Mar 29, 2022

It turns out that most of this problem was due to performance issues with the plugin mapbox-gl-draw-geodesic. These have now been fixed in V2.1.3, so I'm closing this issue even though there's an easy draw perf improvement in draw itself by reducing the JSON.parse/JSON.stringify calls as outlined above.

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

No branches or pull requests

1 participant