search

farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
62 stars 39 forks source link

Use geolocation to associate logs with areas #98

Closed alexadamsmith closed 5 years ago

alexadamsmith commented 5 years ago

We could suggest that logs be associated with areas based on the device's physical location. If the user is located inside an area and creates a log, that area could be selected by default in the 'new log' view. The user could select a different area, or no area, if they prefer.

Cordova has a plugin to access device geolocation. I haven't used it, but it MAY be straightforward! I can imagine an asynch process that's triggered when the user enters the new record view that goes something like

@mstenta identified a library called turf.js that could help us check for intersection: https://gis.stackexchange.com/questions/147829/how-to-check-for-geometry-intersection-point-in-polygon-using-openlayers3

Mike and @jgaehring, I'm thinking this is something I might like to work on in the next little while, since it looks like we can't do much with harvest logs until the quantity problem is sorted out. Do you think it would make sense for me to create a branch and start messing around with geolocation? Or would it be best for me to wait until the current sprints are completed?

mstenta commented 5 years ago

I can't wait to see this take shape! :-)

I'll defer to @jgaehring on overall repo strategy - but I'm sure you could at least start testing the Cordova geolocation features to see what they offer. Hopefully they do what we want!

jgaehring commented 5 years ago

I'm psyched to see this happen too! I think it's a great project to work on for the time being, since it would be a smaller component within the existing Logs.vue container component. I think the harvest log will benefit directly from the work I'm doing with the app shell in Sprint 3, since that will make it possible to switch between pages—something we can't do currently. Not that you couldn't start on the harvest log on another branch if you wanted; we just might not be able to merge it into master for a while, and it might also be hard to test out on a device.

Generally speaking, you should totally feel free to experiment on local branches. I'd only ask you hold off from pushing them to GitHub until there were features on a particular branch we wanted to merge into master. That way we keep the main repo tidy.

One suggestion I might make, too, while you're working on any such project, is to open a branch on each repo (native and client) to keep track of the specific version your using for each. Even if you're only intending to make changes to one, this will help prevent you from accidentally pulling breaking changes I might make to master while you're working from your version. I'm hoping to merge the repos back together after Sprint 3, so hopefully this won't be an issue for too much longer.

alexadamsmith commented 5 years ago

OK, that sounds good! The native-and-client-branch strategy makes a lot of sense. I'll start working on geolocation within the next week, keep it local for now, and let you know when I've got something worth sharing.

jgaehring commented 5 years ago

Sounds good!

One other thing, regarding the Cordova plugin... If you can, do a little investigating into how similar its API is to regular web API's. The MDN docs will provide a good reference. It would be nice if the native version didn't have to be anything different from the web version, and whatever the Cordova plugin needed could just be confined to the config.xml. If there are differences, however, that would be a good reason to confine the API details to a separate module, kind of like I've done the camera in camModule.

alexadamsmith commented 5 years ago

I'm starting up a local branch to work on this, and wanted to check in really quick about how areas are stored locally. Right now, it looks like updateAreas in httpModule pulls areas from the server, then reduces them down to tid and name before sending them to the DB and loading them into app state. So as things stand, I don't have any geographical data to work with.

I was looking at how areas are referenced and used throughout the app, and it seems like full-fledged areas could serve all the same roles as the current, slimmed-down version, without requiring changes to the logic. So on my local branch, I'm going to remove the portion of httpModule line 31 that reduces areas down to their tid and name.

If this seems like a bad way to do it, just let me know and I can work out another way of getting at the full area objects.

mstenta commented 5 years ago

The farm_sync module may be able to provide some guidance on this - since it's main purpose is to sync area data, including name, id, type, and geometry (just those 4 currently).

You can see it doing that here: https://github.com/mstenta/farm_sync/blob/8.x-1.x/farm_sync.module#L121

Notice that it pulls the geometry from $record['field_farm_geofield'][0]['geom'] on the record returned from the farmOS API. That will be in "Well-Known Text" format.

I'll defer to @jgaehring on whether or not it makes sense to "remove the portion of httpModule line 31 that reduces areas down to their tid and name". But it seems that you could also just add a bit of code to add a "geom" property to the area records that are stored locally. I imagine part of the reason they are being stripped down to bare minimum is to reduce the amount of memory they take up. There is probably a lot of unnecessary cruft in the area records returned from the API that would not be necessary to store locally in the app db.

jgaehring commented 5 years ago

Those update actions might actually need some overhaul. I'm just looking at them now in relation to https://github.com/farmOS/farmOS-native/issues/76. So they're not so precious we can't muddle around with them; likely we'll have to sort some stuff out at merge time regardless.

But I am in favor of only adding the properties we need, so as not to have too much data in the cache, at least until we see how it performs. If you just add the geometry fields at lines 31, 34 and 36, would that be enough?

alexadamsmith commented 5 years ago

What you're saying makes sense. For now I'll just add the geom property and leave everything else the same.

Also, I just discovered that I am unable to run the most recent commits. When I pull the most recent changes from farmOS-native and farmOS-client, I'm able to build to the dev server, but the app is unable to communicate with my farmOS installation on docker. For every http call to the server, I get a 404 error: failed to load resource at :8080/null/whatever_resource.
My farmOS docker installation is running at localhost , and earlier versions of the app were able to direct their calls to it just fine.

I tried stepping back commits to farmOS-native to see where the problem arises. The oldest commit with problems is dea748f5a532a6caeceba85942e23460429e5f5d (jan 15; fix bug with localstorage). When I go one commit back, things work fine. So right now I'm using the most recent version of farmOS-client, and commit f42daae18194c5182e9b9af0ad5d111f19f9f57f (jan 15; fix bug with areas) for farmOS-native. For now I'll just do my local work on top of that.

I should also note that the 404 errors only occur when I clear my browser cache. If I move from f42d up to dea7 but don't clear my cache, everything keeps working.

jgaehring commented 5 years ago

Thanks for catching that bug, @alexadamsmith . It should be resolved by https://github.com/farmOS/farmOS-native/commit/5169c3d8d247cc52c042c1c802556308c3e11b1e

alexadamsmith commented 5 years ago

Yep - I rebased my feature branch to the latest commit, and now it works great!

alexadamsmith commented 5 years ago

I just pushed a feature branch called 'geolocate' to both farmOS-native and farmOS-client. It uses the cordova geolocation plugin and turf.js library to check if the device is located inside each area that has a polygon associated with it. The action happens in geoModule, in farmOS-native/src/data.

Jamie, you'll be happy to know that the geolocation plugin also returns the geolocation when running in the browser. So the geoModule will work fine in either a web app or Cordova app, with no changes needed to the code.

The current setup works for areas consisting of a single polygon. However, it does NOT work for areas consisting of several polygons. I had to build a converter to turn farmOS polygons into the geoJSON format that turf uses. Upgrading it to work with farmOS geometry collections will be my next step.

Right now I'm also importing the entire turf library, which is probably not necessary. With turf it's possible to import only the modules you need, and I plan to pare it down. On a similar note, when saving areas to the local database I'm currently saving the entire field_farm_geofield, rather than just the polygon or geometry collection. I also plan to change this in the future.

I feel that the basic structure and function of the feature are workable right now, so if you want to mess around with integrating it into the UI, feel free! I'd also be glad to give a walkthrough of the code any time.

alexadamsmith commented 5 years ago

geoModule now accommodates geometry collections as well as simple polygons.

mstenta commented 5 years ago

Awesome!

FYI @alexadamsmith here is what it would look like (in JSON) to save a GPS coord to the log geofield (per our discussion today):

"field_farm_geofield":[{"geom":"POINT (-95.612887144089 42.797329422858)"}]

So it would be super easy to add the current location to logs via the app! That would be really nice!

You may want to start a separate issue for it, though, and think about whether or not it should be optional (eg, with a checkbox that says "Record current location to the log" or something, that defaults to checked). I can imagine situations where you may want to uncheck it.

alexadamsmith commented 5 years ago

Great - that will be no trouble at all!

Good thinking about making the geolocation optional. Sometimes you'll want to log an observation made previously, and your current location won't be relevant. Also, some people appreciate the choice to opt out of being physically tracked.

We might even include it as a setting, when we have settings.

On Tue, Jan 22, 2019 at 2:59 PM Michael Stenta notifications@github.com wrote:

Awesome!

FYI @alexadamsmith https://github.com/alexadamsmith here is what it would look like (in JSON) to save a GPS coord to the log geofield (per our discussion today):

"field_farm_geofield":[{"geom":"POINT ((-95.612887144089 42.797329422858)"}]

So it would be super easy to add the current location to logs via the app! That would be really nice!

You may want to start a separate issue for it, though, and think about whether or not it should be optional (eg, with a checkbox that says "Record current location to the log" or something, that defaults to checked). I can imagine situations where you may want to uncheck it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-456541606, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8IiIysUJqH5DOzGaSzZuqgiZJqMHks5vF228gaJpZM4Z5ox3 .

mstenta commented 5 years ago

Sometimes you'll want to log an observation made previously, and your current location won't be relevant.

Yes! That's true too. There may also be cases where you want to record the location when you start the log, but then you want to edit the log later on (from somewhere else) without affecting that original location. We don't have the ability to edit logs in the app yet, but when we do...

Ideally it would show the point on a map and let you edit it, but that's a lot more work. Baby steps... :-)

jgaehring commented 5 years ago

I think my vote would go to geolocation OFF by default. I often keep GPS shut off on my phone entirely, primarily for battery life, and it is always annoying to me when app's insist that I turn it on. If I request a specific feature that requires GPS, I'm ok with it asking me for permission to turn GPS on. But beyond that it's annoying, like when I just want to check the temperature and the weather app insists I activate location settings.

jgaehring commented 5 years ago

@alexadamsmith , do you feel you have a feature that's ready for opening a PR, or do you want to get some feedback before moving on to developing the UI for it?

mstenta commented 5 years ago

I think my vote would go to geolocation OFF by default.

Yea good points.

Maybe a simple button "Add current location (GPS)" that you can press to add the GPS coords, with a "Delete" or "X" to remove it (similar to asset references). This would allow users to add it with a single click, and remove/omit it as well.

jgaehring commented 5 years ago

A radio button might be nice.

alexadamsmith commented 5 years ago

Yes, I think either a simple button or a radio button would be great. So the app would not be requesting geolocation by default; it would only do so on demand. One thing to consider - I've noticed that the browser takes several seconds to return geolocation. I need to test this more on an actual native device to see how long it takes. If geolocation takes a while to return, that could make the button option clunky.

As to the push request, I think I'd like to fiddle with this a little bit more before rolling it out. I want to think over when it make sense to request geolocation, and also see if I can slim down the area geometry field to include only the data we need. But it should be in good shape within the next few days.

On Tue, Jan 22, 2019 at 8:01 PM Michael Stenta notifications@github.com wrote:

I think my vote would go to geolocation OFF by default.

Yea good points.

Maybe a simple button "Add current location (GPS)" that you can press to add the GPS coords, with a "Delete" or "X" to remove it (similar to asset references). This would allow users to add it with a single click, and remove/omit it as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-456625498, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8FQrOK1HMYNNxy9CRo67SDzNNsO2ks5vF7RwgaJpZM4Z5ox3 .

mstenta commented 5 years ago

All this talk raises the question: what triggers the geolocation in this branch currently? Is it automatic? Or does the user have to click something? If automatic, do we want that to be manual as well? Or is that what you're also thinking about with this discussion? (Just trying to separate the two features: geolocating for finding the area you're in vs geolocating to add GPS to the log geofield.)

alexadamsmith commented 5 years ago

Currently the branch gets geolocation on app load (it uses the DeviceReady hook in App.vue). That's not going to be a long-term sustainable place for it, because if you've had the app open for a little while, you will have a stale location.

Mike makes a good point, and I agree we need to separate the question whether and when the app access device geolocation, from the question of whether geolocation is saved in a record. Here are my thoughts:

I plan to play around with this some tomorrow.

Thank you both for the super helpful input on this feature!!

Alex

On Wed, Jan 23, 2019 at 9:19 AM Michael Stenta notifications@github.com wrote:

All this talk raises the question: what triggers the geolocation in this branch currently? Is it automatic? Or does the user have to click something? If automatic, do we want that to be manual as well? Or is that what you're also thinking about with this discussion? (Just trying to separate the two features: geolocating for finding the area you're in vs geolocating to add GPS to the log geofield.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-456817772, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8AxKd0gg2Coz7P6sZ1JGaYsP5WN7ks5vGG9ngaJpZM4Z5ox3 .

jgaehring commented 5 years ago

I think it's good to think back to the user's intentions, and how those intentions effect their interactions with these features.

When it comes to suggesting areas, the user first has to make the decision whether they want to select an area at all. Otherwise, none of this even matters to the user. If they do, they'll then make a decision how to select an area, whether to search all areas, or use their own location. Finally, they'll execute on that decision, either selecting their choice from search results, or verifying their location (perhaps from a list of the nearest locations, or something like that).

By default, I suggest showing a view with a search field, with a radio button for toggling between search and geolocation:

img_20190123_204311

Then, only if the user toggles the selection, show:

img_20190123_204913

Only once the user makes that choice do you trigger the geolocation API. That way you're not doing so prematurely (resulting in stale data) or without the users choice.

The field for saving the location with the log will be a separate choice, so should have a separate trigger. These could share methods and actions, even perhaps state in the store, but the processes should be separate and triggered by separate user interactions. The actual interface for saving the location could be much simpler, just a single radio button, defaulting to unselected.

jgaehring commented 5 years ago

Oh btw, @alexadamsmith , if you think of more things for the settings panel, I'm starting to work on it now, tracking it in https://github.com/farmOS/farmOS-client/issues/26. Feel free to add suggestions there.

mstenta commented 5 years ago

Good thoughts @alexadamsmith and @jgaehring - here's my attempt at synthesizing them:

The actual interface for saving the location could be much simpler, just a single radio button, defaulting to unselected.

Do you mean checkbox? You can't unselect a single radio once it's been selected.

alexadamsmith commented 5 years ago

Hey all - I'm absolutely in favor of Mike's proposed synthesis.

If we don't yet need a settings view for other reasons, I wouldn't create one just to deal with geolocation. We can default to the 'search areas' rater than the 'use my location' radio. The geolocation plugin would not be called until the user either chooses 'use my location' or clicks the 'Add my current GPS location to log' button. I've tested the app on my phone, and it takes about two seconds from the time the plugin is called to return geolocation.

As a first crack at this, I think it will be OK to have 'use my location' and 'search areas' as an either/ or choice, rather than allowing both. If someone needs to associate a log with multiple areas, they can just use the 'search areas' option.

One other thing I want to point out: the current geoModule does not search for nearby areas; it just checks whether you are currently inside an area. The ability to suggest nearby areas would be super handy, and I know turf has tools for that. But right now, our functionality may be a bit more limited.

I was going to mess around with a basic UI implementation today, and maybe get the feature branch to a place where it can be merged in.

On Fri, Jan 25, 2019 at 2:05 PM Michael Stenta notifications@github.com wrote:

Good thoughts @alexadamsmith https://github.com/alexadamsmith and @jgaehring https://github.com/jgaehring - here's my attempt at synthesizing them:

  • Settings form that enables GPS use app-wide. If users don't want to share location, this can be disabled (and is probably disabled by default).
  • If the setting is enabled, present the two radio options that @jgaehring https://github.com/jgaehring suggested in his whiteboard drawings above. If the setting is not enabled, do not show the radios, and only show the search field.
    • Important: when we add the ability to EDIT logs, this will need to behave accordingly. We do NOT want log areas to be overwritten accidentally. Perhaps the best approach is to default to the search field no matter what, populated with whatever areas are already referenced.
    • Side question: what if you want to be able to do both? eg: Attaching multiple areas, but you want to find an area AND select a different area from search? Do we need to separate the "selection widget" from the "list of attached areas", so that both widgets just add to the list, but the list works the same (eg: shows referenced areas and allows you to delete them)?
  • Button to "Add my current GPS location to log" that you have to manually click, along with the ability to "remove" it if you click it by mistake, or some other reason.

The actual interface for saving the location could be much simpler, just a single radio button, defaulting to unselected.

Do you mean checkbox? You can't unselect a single radio once it's been selected.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-457686888, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8OrNlQpLHfPXwOQn89i4Ek2GnX9Bks5vG1WJgaJpZM4Z5ox3 .

jgaehring commented 5 years ago

Important: when we add the ability to EDIT logs, this will need to behave accordingly. We do NOT want log areas to be overwritten accidentally.

I don't think this will be a problem, if I understand you. The way editing will work is essentially to move the HEAD (currentLogIndex in the store) to the desired log's index number. Properties are written, individually, to that log on input of a specific field; it's not like a regular 'submit' process where you submit the whole form and all its fields each time, if that's what you're worried about?

Do you mean checkbox? You can't unselect a single radio once it's been selected.

Oops, I guess I was thinking more of a toggle, not a radio button. Sorry for not specifying. This is what I pictured: image

which I guess is in fact just a glorified checkbox.

If we don't yet need a settings view for other reasons, I wouldn't create one just to deal with geolocation.

It's already being built! You can't see the animation here, but I've got it working pretty smoothly:

image

Now I'm just debating whether to try to implement swipe gestures, or pull in Hammer.js to handle it... :thinking:

One other thing I want to point out: the current geoModule does not search for nearby areas; it just checks whether you are currently inside an area. The ability to suggest nearby areas would be super handy, and I know turf has tools for that. But right now, our functionality may be a bit more limited.

I'm thinking it could be really desirable, perhaps even necessary, to search for nearby areas, just because I don't know how accurate we can expect GPS to be, and because I can see a LOT of use cases where the user is standing just at the edge of a boundary line, maybe just inside it or outside it, rather than smack in the middle. I'm imagining someone trying to get their location, and it keeps coming back with zero results because they're just a couple yards outside the perimeter as it's mapped in farmOS; then they have to walk around a bunch just to get the reading they expected. If turf has easy enough tools to do this, I'd vote for trying to implement this before merging into master. Otherwise, perhaps we could do some test builds with this on Android to see if this is really a problem? I guess another option is to calculate the boundaries taking into account some reasonable margin for error? What are your guys' thoughts on this?

mstenta commented 5 years ago

it's not like a regular 'submit' process where you submit the whole form and all its fields each time, if that's what you're worried about?

Ok yea, maybe not an issue. My concern was around how "default form values" work in the app - and just making sure it wasn't possible to overwrite the areas that are already saved to a log accidentally. Eg: you save a log in farmOS server (not the mobile app) with an area reference, then sync it into the app, open it up in the app to edit the log: the area reference field (and whatever options we provide for using location) need to be able to maintain areas that already exist, so they are not lost. This may not be an issue at all, just came to mind so I mentioned it. :-)

Oops, I guess I was thinking more of a toggle, not a radio button.

Gotcha gotcha. Makes sense. A boolean field, basically. Love the toggle widget! Much better than a checkbox.

It's already being built! You can't see the animation here

Ooh la la! :-D

Otherwise, perhaps we could do some test builds with this on Android to see if this is really a problem? I guess another option is to calculate the boundaries taking into account some reasonable margin for error? What are your guys' thoughts on this?

These strike me as two commits, in terms of development, and I'd love to test it out to see how accurate my phone's GPS is as a first step! So if it makes sense to do the simple approach first and test it, I'm all for it. :-)

@alexadamsmith question: where is the checkInside function? I see it referenced in this commit, but I'm not sure where it's defined: https://github.com/farmOS/farmOS-client/commit/31e1eb3070713c1e2424d4537bd653d735651f62 - Just curious how it works.

I see that turf.js has an intersect function: http://turfjs.org/docs/#intersect - It takes two polygons, and returns null if they don't intersect at all.

If you are starting with a GPS coordinate (a point, not a polygon), one option would be to blow out the point into a circle with N radius (maybe a setting in the app, with a sane default that corresponds to what the accuracy of a typical phone GPS chip provides). Then you would have two polygons you could compare - AND it would give you some "nearby" detection as well (with configuration for fuzziness).

alexadamsmith commented 5 years ago

Jamie, I think you make a good point about the usefulness of identifying nearby areas. Right now I've got the check-inside functionality with basic UI elements up and running on my local branch. What I'm thinking I need to do is

Talk to you soon!

Alex

On Mon, Jan 28, 2019 at 9:49 AM Jamie Gaehring notifications@github.com wrote:

Important: when we add the ability to EDIT logs, this will need to behave accordingly. We do NOT want log areas to be overwritten accidentally.

I don't think this will be a problem, if I understand you. The way editing will work is essentially to move the HEAD (currentLogIndex in the store) to the desired log's index number. Properties are written, individually, to that log on input of a specific field; it's not like a regular 'submit' process where you submit the whole form and all its fields each time, if that's what you're worried about?

Do you mean checkbox? You can't unselect a single radio once it's been selected.

Oops, I guess I was thinking more of a toggle, not a radio button. Sorry for not specifying. This is what I pictured: [image: image] https://user-images.githubusercontent.com/5556545/51841923-b74e7580-22dd-11e9-83bb-e67fbb9b1af8.png

If we don't yet need a settings view for other reasons, I wouldn't create one just to deal with geolocation.

It's already being built! You can't see the animation here, but I've got it working pretty smoothly:

[image: image] https://user-images.githubusercontent.com/5556545/51843675-82dcb880-22e1-11e9-8f51-c570ed752b2c.png

Now I'm just debating whether to try to implement swipe gestures, or pull in Hammer.js https://hammerjs.github.io/ to handle it... 🤔

One other thing I want to point out: the current geoModule does not search for nearby areas; it just checks whether you are currently inside an area. The ability to suggest nearby areas would be super handy, and I know turf has tools for that. But right now, our functionality may be a bit more limited.

I'm thinking it could be really desirable, perhaps even necessary, to search for nearby areas, just because I don't know how accurate we can expect GPS to be, and because I can see a LOT of use cases where the user is standing just at the edge of a boundary line, maybe just inside it or outside it, rather than smack in the middle. I'm imagining someone trying to get their location, and it keeps coming back with zero results because they're just a couple yards outside the perimeter as it's mapped in farmOS; then they have to walk around a bunch just to get the reading they expected. If turf has easy enough tools to do this, I'd vote for trying to implement this before merging into master. Otherwise, perhaps we could do some test builds with this on Android to see if this is really a problem? I guess another option is to calculate the boundaries taking into account some reasonable margin for error? What are your guys' thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-458160433, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8FSIcxRrfBy_sJ-GlJDZ9Ick0fIyks5vHw3pgaJpZM4Z5ox3 .

mstenta commented 5 years ago

one option would be to blow out the point into a circle with N radius

Looks like the turf.js library has a function for this: http://turfjs.org/docs/#circle

alexadamsmith commented 5 years ago

checkInside is in data -> geoModule in the farmOS-native repository. That's a good suggestion RE the circle! But I think there are already some proximity functions in turf.. I'll explore

On Mon, Jan 28, 2019 at 10:19 AM Michael Stenta notifications@github.com wrote:

it's not like a regular 'submit' process where you submit the whole form and all its fields each time, if that's what you're worried about?

Ok yea, maybe not an issue. My concern was around how "default form values" work in the app - and just making sure it wasn't possible to overwrite the areas that are already saved to a log accidentally. Eg: you save a log in farmOS server (not the mobile app) with an area reference, then sync it into the app, open it up in the app to edit the log: the area reference field (and whatever options we provide for using location) need to be able to maintain areas that already exist, so they are not lost. This may not be an issue at all, just came to mind so I mentioned it. :-)

Oops, I guess I was thinking more of a toggle, not a radio button.

Gotcha gotcha. Makes sense. A boolean field, basically. Love the toggle widget! Much better than a checkbox.

It's already being built! You can't see the animation here

Ooh la la! :-D

Otherwise, perhaps we could do some test builds with this on Android to see if this is really a problem? I guess another option is to calculate the boundaries taking into account some reasonable margin for error? What are your guys' thoughts on this?

These strike me as two commits, in terms of development, and I'd love to test it out to see how accurate my phone's GPS is as a first step! So if it makes sense to do the simple approach first and test it, I'm all for it. :-)

@alexadamsmith https://github.com/alexadamsmith question: where is the checkInside function? I see it referenced in this commit, but I'm not sure where it's defined: 31e1eb3 https://github.com/farmOS/farmOS-client/commit/31e1eb3070713c1e2424d4537bd653d735651f62

  • Just curious how it works.

I see that turf.js has an intersect function: http://turfjs.org/docs/#intersect - It takes two polygons, and returns null if they don't intersect at all.

If you are starting with a GPS coordinate (a point, not a polygon), one option would be to blow out the point into a circle with N radius (maybe a setting in the app, with a sane default that corresponds to what the accuracy of a typical phone GPS chip provides). Then you would have two polygons you could compare - AND it would give you some "nearby" detection as well (with configuration for fuzziness).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-458172392, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8CQHqWFW4gaVNiYow5og1FB_ih2_ks5vHxUYgaJpZM4Z5ox3 .

mstenta commented 5 years ago

checkInside is in data -> geoModule in the farmOS-native repository.

Ah gotcha! Forgot to check farmOS-native. :-)

If you want to try the circle approach, I think you would just need to change this:

      const point = turf.point(params.point);
      const polygon = turf.polygon(polyJSON);
      const isInside = turf.inside(point, polygon);

to this:

      const circle = turf.circle(params.point, 0.001, {units: 'kilometers'});
      const polygon = turf.polygon(polyJSON);
      const isInside = turf.intersect(circle, polygon) !== null;

That will make a circle with a radius of 1 meter (diameter of 2 meters), which should be good for most GPS chips in phone.

(You may also want to rename the isInside variable, but the above should work.)

alexadamsmith commented 5 years ago

Cool - let me give that a try! By the way, I just pushed my current working commits to the feature branch. They're rebased to make them current with the master branch. If the radius approach works, then all I'll have to to do is accommodate multiple results from the geolocation search.

On Mon, Jan 28, 2019 at 10:44 AM Michael Stenta notifications@github.com wrote:

checkInside is in data -> geoModule in the farmOS-native repository.

Ah gotcha! Forgot to check farmOS-native. :-)

If you want to try the circle approach, I think you would just need to change this:

  const point = turf.point(params.point);
  const polygon = turf.polygon(polyJSON);
  const isInside = turf.inside(point, polygon);

to this:

  const radius = 0.001;
  const circle = turf.circle(params.point, radius, {units: 'kilometers'});
  const polygon = turf.polygon(polyJSON);
  const isInside = turf.intersect(circle, polygon) !== null;

That will make a circle with a radius of 1 meter (diameter of 2 meters), which should be good for most GPS chips in phone.

(You may also want to rename the isInside variable, but the above should work.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-458181969, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8KFnjkG9gwD8gOAClqlw5J0HUfDgks5vHxrLgaJpZM4Z5ox3 .

alexadamsmith commented 5 years ago

Just FYI, turf.intersect checks whether the boundaries of two polygons intersect. So if I have a circle that falls entirely inside a polygon, turf.intersect returns null! So I just used both turf.intersect and turf.inside:

  const radius = 0.01;
  const circle = turf.circle(params.point, radius, { units:

'kilometers' }); const polygon = turf.polygon(geomJSON); const isInside = turf.inside(params.point, polygon); const isNear = turf.intersect(circle, polygon) !== null; if (isInside || isNear) { commit('addLocalArea', params.area) }

For starters, I think I'll go with a radius of 10m rather than 1m in case someone is standing by their field rather than inside it. This would be a useful thing to adjust in settings!

On Mon, Jan 28, 2019 at 10:56 AM Alex Smith alexadamsmith@gmail.com wrote:

Cool - let me give that a try! By the way, I just pushed my current working commits to the feature branch. They're rebased to make them current with the master branch. If the radius approach works, then all I'll have to to do is accommodate multiple results from the geolocation search.

On Mon, Jan 28, 2019 at 10:44 AM Michael Stenta notifications@github.com wrote:

checkInside is in data -> geoModule in the farmOS-native repository.

Ah gotcha! Forgot to check farmOS-native. :-)

If you want to try the circle approach, I think you would just need to change this:

  const point = turf.point(params.point);
  const polygon = turf.polygon(polyJSON);
  const isInside = turf.inside(point, polygon);

to this:

  const radius = 0.001;
  const circle = turf.circle(params.point, radius, {units: 'kilometers'});
  const polygon = turf.polygon(polyJSON);
  const isInside = turf.intersect(circle, polygon) !== null;

That will make a circle with a radius of 1 meter (diameter of 2 meters), which should be good for most GPS chips in phone.

(You may also want to rename the isInside variable, but the above should work.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/35#issuecomment-458181969, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8KFnjkG9gwD8gOAClqlw5J0HUfDgks5vHxrLgaJpZM4Z5ox3 .

mstenta commented 5 years ago

Just FYI, turf.intersect checks whether the boundaries of two polygons intersect. So if I have a circle that falls entirely inside a polygon, turf.intersect returns null!

Oh wow, that sounds like a bug in the turf.js library - or at least an undocumented limitation.

mstenta commented 5 years ago

So I just used both turf.intersect and turf.inside:

Makes sense!

alexadamsmith commented 5 years ago

OK, I went ahead and made pull requests for the geolocate branches. Unfortunately I've not been able to get eslint to work in my client repository, so I'm sure there are some formatting errors that I missed in there.

jgaehring commented 5 years ago

I think we can close this, right @alexadamsmith ? Are we still waiting on any additional functionality? @mstenta and I talked about the ability to delete geolocation points after they've been added, but perhaps that could be its own issue at this point.

alexadamsmith commented 5 years ago

Yep, we're done with this for now

mstenta commented 5 years ago

(Transferring all issues from old repository. See https://github.com/farmOS/farmOS-native/issues/92)