search

farmOS / farmOS-map

farmOS-map is an OpenLayers wrapper library designed for agricultural mapping needs. It can be used in any project that has similar requirements.
https://farmOS.github.io/farmOS-map
MIT License
40 stars 22 forks source link

Snapping grid integration #79

Closed symbioquine closed 4 years ago

symbioquine commented 4 years ago

Following up from https://github.com/farmOS/farmOS-map/issues/68#issuecomment-632220884, this issue will track the potential integration of a snapping grid feature for farmOS-map.

I've pulled the logic for actually drawing the grid from https://github.com/symbioquine/farm_map_snapping_grid out into https://github.com/symbioquine/ol-grid

Before proceeding with the implementation of the new feature, I'd like feedback on a few points;

farm_map_snapping_grid_changing_dimensions_and_units

mstenta commented 4 years ago

Awesome @symbioquine! Great idea to make it into a general purpose OL library. :-)

What would the best style for the grid points? For farm_map_snapping_grid I went with small green dots. Is there a reason to prefer something else?

I like the little green dots. Matches the overall color scheme of farmOS-map. Wonder if you want that to be configurable in ol-grid? (Or maybe it already is?)

What would the best location for the grid controls be? For farm_map_snapping_grid I went with some buttons near the scale bar. Is there a reason to prefer something else?

I like this too. Can't think of a better place - but we can always rethink it in the future. As good a place to start as anywhere!

Is there a better arrangement/paradigm for keeping the grid controls from cluttering the screen? With farm_map_snapping_grid, I went with a expandable/collapsible sort of drawer but I'm sure there must be a better approach - I'm no UX designer :)

This seems fine to me. The only other thoughts I have are: this feature is useful in specific instances (eg: drawing beds), but not everyone needs that, and those who do only need it from time to time (eg: when drawing beds, which generally will only be done once). So I wonder how we can add this only in contexts where it is needed. And also: how do we make it clear what it is and how to use it?

Similarly: do we keep it in it's own module within farmOS, so that it can be enabled only on farms that need it?

symbioquine commented 4 years ago

@mstenta I've published an initial version of this integration in https://github.com/farmOS/farmOS-map/pull/81. Looking forward to any feedback.

mstenta commented 4 years ago

@symbioquine I finally got around to playing with PR #81 - it looks/works great!!

Couple of quick thoughts:

Can we decouple the snappingGridFeature from the Edit control? I wonder if it could be made to work more like the measure behavior... which is added to a map with a reference to the layer it should apply to: https://github.com/farmOS/farmOS-map/blob/0f855d36495f0e694e7007a6b01b8dcdd7aec9ee/static/index.html#L33

This would allow it to be added to maps selectively, and would keep the concerns separated from the Edit code. I haven't looked closely enough to see how tightly coupled they are, or how feasible that would be, though, so curious to hear your thoughts.

Or... maybe that's not really possible because the drawing controls need to snap to the snapping grid?

Last thought: any plans to publish ol-grid to NPM? Seems like a logic next step!

symbioquine commented 4 years ago

Thanks for looking at this @mstenta!

Can we decouple the snappingGridFeature from the Edit control? I wonder if it could be made to work more like the measure behavior... which is added to a map with a reference to the layer it should apply to:

https://github.com/farmOS/farmOS-map/blob/0f855d36495f0e694e7007a6b01b8dcdd7aec9ee/static/index.html#L33

This would allow it to be added to maps selectively, and would keep the concerns separated from the Edit code. I haven't looked closely enough to see how tightly coupled they are, or how feasible that would be, though, so curious to hear your thoughts.

Good point. Originally I had them coupled because I was adding the grid geometry into the Edit control's snap interaction. However, in order to have a different snap tolerance, I switched to using a separate snap interaction. The only reason I kept the coupling at that point was that the book-keeping required to manage when the snap interaction is added to the map was slightly more complex when implemented independently of the Edit control's state.

I've now implemented that book-keeping directly in the SnappingGrid control; https://github.com/farmOS/farmOS-map/pull/81/commits/b5030279e3560f609db619078d4790b4d7b76d17#diff-4ec328646e852ef108607ed4c429ff00R309-R357

I've published a new PR with the changes squashed since a lot of the intermediate commits would just be noise in the farmOS-map commit history.

Last thought: any plans to publish ol-grid to NPM? Seems like a logic next step!

Agreed. I'll probably pursue that sometime soon, but - if possible - I'd prefer if it didn't block getting this feature into farmOS-map...

mstenta commented 4 years ago

Wow great!! Wasn't sure how complicated decoupling would be. Nice job!

And I agree that NPM ol-grid doesn't need to hold this up. :-)

I copied this branch to my fork and rebased it on top of master. Then I added a commit for your review: https://github.com/mstenta/farmOS-map/commit/926e2496f8d9db7e3461ebe2c0306268523ecb83

What it does is removes the SnappingGrid from the default controls, and adds a snappingGrid behavior, which can be added to maps via instance.addBehavior('snappingGrid'). I added that to index.html after instance.addBehavior('edit').

This provides a bit more flexibility about when the snapping grid can be added. So it would be possible to build a map with the Edit control but without the SnappingGrid control, if that were desired. What do you think? Make sense?

mstenta commented 4 years ago

Oh the last thing we'll need is documentation! It would be good to document both a) how to add the snappingGrid behavior to a map (as described above), as well as b) how to use the snapping grid! Perhaps just including the animated demo you made in the project README.md would be enough?

mstenta commented 4 years ago

Oh and once this is merged... we should also make a complimentary PR in farmOS to add the snapping grid everywhere that the edit behavior is used. There are a few, and I think it would make sense to add the snapping grid to all of them, but if there are any that wouldn't benefit from it we can leave it out.

symbioquine commented 4 years ago

Thanks @mstenta!

What it does is removes the SnappingGrid from the default controls, and adds a snappingGrid behavior, which can be added to maps via instance.addBehavior('snappingGrid'). I added that to index.html after instance.addBehavior('edit').

This provides a bit more flexibility about when the snapping grid can be added. So it would be possible to build a map with the Edit control but without the SnappingGrid control, if that were desired. What do you think? Make sense?

That makes sense since my strategy of putting it in the default controls would show the SnappingGrid button in some read-only map cases where it probably isn't relevant/helpful.

The change in my PR would still allow the SnappingGrid to be turned off by customizing the default Controls (roughly) as described in the Readme e.g.

farmOS.map.create(id, {
  controls: (defaults) => defaults.filter(def => (
    // Using the control css class name for filtering since the def.constructor.name
    // strategy the readme describes doesn't work after the default Webpack minification
    // (I'll open a separate issue for that.)
    !def.element.classList.contains("ol-snapgrid")
  )),
});

We could try to make it smarter where it is hidden - or hides itself - based on the presence of other edit controls, but I think your proposal is probably the simplest/best path forward.

Oh and once this is merged... we should also make a complimentary PR in farmOS to add the snapping grid everywhere that the edit behavior is used. There are a few, and I think it would make sense to add the snapping grid to all of them, but if there are any that wouldn't benefit from it we can leave it out.

Makes sense.

Oh the last thing we'll need is documentation! It would be good to document both a) how to add the snappingGrid behavior to a map (as described above), as well as b) how to use the snapping grid! Perhaps just including the animated demo you made in the project README.md would be enough?

Both a/b sound good. I can put a little info on usage into the Readme, but in my mind most of the usage documentation might be better located as a section under https://farmos.org/guide/areas/ (also as a follow-up PR) does that make sense?

mstenta commented 4 years ago

Great!! Yes usage documentation on farmOS.org makes the most sense, agreed!

symbioquine commented 4 years ago

I've updated the current PR to include the rebased changes and the Readme update.

Next up: farmOS & farmOS.org PR's... :)

symbioquine commented 4 years ago

Here's the PR for farmOS; https://github.com/farmOS/farmOS/pull/335

symbioquine commented 4 years ago

And, finally, the PR for farmOS.org; https://github.com/farmOS/farmOS.org/pull/96

mstenta commented 4 years ago

@symbioquine Awesome!!

OK, so I think this is the order of events:

  1. ~Merge the farmOS-map PR: https://github.com/farmOS/farmOS-map/pull/85~
  2. ~Tag and release farmOS-map v1.3.0~
  3. ~Update farmOS-map to v1.3.0 in farmOS's drupal-org.make file: https://github.com/farmOS/farmOS/blob/8e52d36315860d497e4f63c63325dd914750381d/drupal-org.make#L192~
  4. ~Review/merge farmOS PR: https://github.com/farmOS/farmOS/pull/335~
  5. ~Create a 7.x-1.5 branch of the farmOS.org repository, to be merged after the release of farmOS 7.x-1.5.~
  6. ~Review/merge farmOS.org PR into the new 7.x-1.5 branch: https://github.com/farmOS/farmOS.org/pull/96~
mstenta commented 4 years ago

Merged PR #85

Tagged and release v1.3.0: https://github.com/farmOS/farmOS-map/releases/tag/v1.3.0

Updated farmOS-map version in farmOS: https://github.com/farmOS/farmOS/commit/fd6e45ad19156f973932eecff63de7354d6fec14

mstenta commented 4 years ago

Merged farmOS PR: https://github.com/farmOS/farmOS/pull/335

Created a 7.x-1.5 branch on farmOS.org and merged the documentation PR into it: https://github.com/farmOS/farmOS.org/pull/96

mstenta commented 4 years ago

I think that's everything! Thanks again for doing all this @symbioquine ! This a great addition!

symbioquine commented 4 years ago

Thanks @mstenta! I hope this tool proves as central/convenient to others' workflows as it has been for my own use-case.