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

Fix issue snapping to mvt #1171

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Conversation

dbauszus-glx
Copy link
Member

This PR fixes an issue attempting to snap to an MVT layer with zoom level restrictions.

"draw": {
  "polygon": {
    "snap": {
      "layer": "iuc"
    }
  }
}

The snap interaction should attempt to display the layer. Snapping is only possible to loaded features. Adding features to the snap source must be tried. MVT layer have duplicate features add duplicate features would crash this process.

@dbauszus-glx dbauszus-glx added the Bug A genuine bug. There must be some form of error exception to work with. label Mar 5, 2024
@dbauszus-glx dbauszus-glx linked an issue Mar 5, 2024 that may be closed by this pull request
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

@dbauszus-glx
✅ This has resolved the issue when creating a new geometry and trying to snap to a layer.
❌ Editing an existing geometry throws a warning in the console.
image

❓ In looking through old configuration - I see a lot of
layer.snap:true - please can we add a warning to remove this as it doesn't do anything?
layer.snap.layer_key - please can we add a warning to update this to layer.snap.layer.layer_key?

@dbauszus-glx
Copy link
Member Author

snap.true should work. this should snap to the layer itself. might be a bug. i haven't tested editing but only drawing, will need to look into this.

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

Nice @dbauszus-glx

All worked for me!

I have added in additional documentation for the mapview snap interaction and the layer mvt format modules.

I added in the params/properties at the top of the each function and nested them.

eg.

/**
@param layer
@param [layer.srid] - description
@param [layer.tables] - description
*/

Please have a look and let me know what you think.

@dbauszus-glx
Copy link
Member Author

@RobAndrewHurst Looking good. I think just a reference for mapview, style, etc object is enough once these these objects are sufficiently described in place.

@simon-leech
Copy link
Contributor

simon-leech commented Mar 11, 2024

Pushed a commit to 'allow' string assignment of snap: layer_key but warn about incorrect configuration. As this configuration is used currently in multiple places, and we don't want any breaking changes.
image

This is still not working on editing a geometry - should this be a separate PR @dbauszus-glx ?

Copy link

sonarcloud bot commented Mar 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dbauszus-glx dbauszus-glx merged commit 44c3af3 into GEOLYTIX:main Mar 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snap fails on MVT layer with zoom restictions
3 participants