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

Feature/add measure tool #124

Merged
merged 127 commits into from
Jan 29, 2025
Merged

Feature/add measure tool #124

merged 127 commits into from
Jan 29, 2025

Conversation

LDWiegand
Copy link
Collaborator

@LDWiegand LDWiegand commented Apr 18, 2024

Summary

Added a measure tool for distances and areas

Instructions for local reproduction and review

npm run snowbox

@LDWiegand LDWiegand added the enhancement New feature or request label Apr 18, 2024
@LDWiegand LDWiegand self-assigned this Apr 18, 2024
@warm-coolguy warm-coolguy self-assigned this Apr 18, 2024
@warm-coolguy warm-coolguy requested a review from dopenguin April 18, 2024 08:32
@warm-coolguy warm-coolguy force-pushed the feature/add-measure-tool branch from 4304b17 to 9197c6c Compare April 26, 2024 09:10
@warm-coolguy warm-coolguy force-pushed the feature/add-measure-tool branch from 9197c6c to 17e61c2 Compare April 26, 2024 09:13
@warm-coolguy warm-coolguy marked this pull request as ready for review April 29, 2024 06:30
Copy link
Member

@warm-coolguy warm-coolguy left a comment

Choose a reason for hiding this comment

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

With 24e386b gone, there's no documented way on how to subscribe to or otherweise access measurements taken in the map left.

🏓 @dopenguin

@dopenguin
Copy link
Member

With 24e386b gone, there's no documented way on how to subscribe to or otherweise access measurements taken in the map left.

🏓 @dopenguin

I've added the information to the featureCollection cc8de51

However, I've encountered an error that stems from the way the draw interaction works and added a todo for that. If you got a lead on how to fix that, let me know. Otherwise, I'll get my thinking cap out 🎩

🏓 @warm-coolguy

@warm-coolguy
Copy link
Member

With 24e386b gone, there's no documented way on how to subscribe to or otherweise access measurements taken in the map left.
🏓 @dopenguin

I've added the information to the featureCollection cc8de51

However, I've encountered an error that stems from the way the draw interaction works and added a todo for that. If you got a lead on how to fix that, let me know. Otherwise, I'll get my thinking cap out 🎩

🏓 @warm-coolguy

I don't see a simple solution without digging deeper into this. Have you seen the OL example? Maybe there's some indication on how it could be done in another way. (I also kinda like how much simpler the UI is, even though it has less information – do we need per-line measurements?)

I've also noticed an additional bug where, if you draw e.g. a triangle, enter "Edit" mode, and pull from the last corner, a new side will exist that has no measurement on its own.

🏓 @dopenguin

@dopenguin
Copy link
Member

dopenguin commented Jan 28, 2025

With 24e386b gone, there's no documented way on how to subscribe to or otherweise access measurements taken in the map left.
🏓 @dopenguin

I've added the information to the featureCollection cc8de51
However, I've encountered an error that stems from the way the draw interaction works and added a todo for that. If you got a lead on how to fix that, let me know. Otherwise, I'll get my thinking cap out 🎩
🏓 @warm-coolguy

I don't see a simple solution without digging deeper into this. Have you seen the OL example? Maybe there's some indication on how it could be done in another way. (I also kinda like how much simpler the UI is, even though it has less information – do we need per-line measurements?)

I've also noticed an additional bug where, if you draw e.g. a triangle, enter "Edit" mode, and pull from the last corner, a new side will exist that has no measurement on its own.

🏓 @dopenguin

After further digging it concluded that the issue exists because of the fix for modifying snapped features in packages/plugins/Draw/src/store/createInteractions/createModifyInteractions.ts.
This probably doesn't work because there are multiple dragSegments that need to be modified to be able to modify a Polygon.

I tested multiple different scenarios to change the underlying Collection when modifystart is called. This either involved directly changing event.features or changing event.target.features_.
Somehow, changing either of those to only include one feature by a) setting the whole Collection to be new or b) updating the Collection via the provided methods didn't yield any results; all snapped features where still being modified.

When checking how OpenLayers handles this, one example includes the same issue of not being able to un-snap features while another example fixes this by using a Select interaction to be the base for the features that can be modified.

All in all, this outcome does not seem satifying whatsoever. However, I suggest that we go the route of the second example and use a Select interaction for the whole modification process (not only for text). This is not the best solution UX-wise, but at least it is consistent.

I'd like to tackle this NOT as part of this PR as this issue is not new but only has become clear now. We'd also be able to merge #211.
I'll create an issue to change the modification process to work based on a Select interaction and assign it to myself if you agree.

Edit: The measure issue has been fixed as well dbf946f

Copy link
Member

@warm-coolguy warm-coolguy left a comment

Choose a reason for hiding this comment

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

I have found the following additional issues:

  • Draw a line of multiple parts. Edit the line by pulling from the middle of line segments or pulling existing points. Sooner or later, no interaction indication is left on some of the fragments; any fragment (no matter if first/last/somewhere in the middle) may be affected. (I.e. the blue dot goes missing and that line segment is no longer editable.)
  • The same thing can happen on polygons.
  • I was able to "find" that line again on this polygon: orly
    Note: That dot is the mouse interaction indicator. If clicked, the polygon is drastically altered.
  • Since 1km²=1000000m², values are miscalculated: hmmm
    I also recommend using decimal places after larger units (km/ha) until values become larger, i.e. 2 decimal places may be used until 10km length are reached or something like that. I furthermore recommend to always give measurements in metres on the exported GeoJSON since that's a more parsable format. Or maybe that and the string, should it only be for display ... but parsing "1km²" versus "100ha" is annoying to distinguish.

:shipit: @dopenguin According to plan you may change whatever you feel like before the merge. To my understanding, a follow-up PR will clean up the rest.

@dopenguin dopenguin merged commit 10e3d4c into main Jan 29, 2025
4 checks passed
@dopenguin dopenguin deleted the feature/add-measure-tool branch January 29, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants