Anything to add? #8

Open
nofxx opened this Issue Sep 14, 2012 · 23 comments

Comments

Projects
None yet
6 participants
Collaborator

nofxx commented Sep 14, 2012

Every function the old spacial had is here, with the exception of distance, which is delegated to external libraries. And some new cool stuff like bbox and new geometry types.

Now it's time to try and think what can be improved. Waiting for suggestions.

I'm using the Concern pattern in order to have every concern in its own module to avoid a bloated domain class:

In this case, I have to define the Point constant directly in my included block. Just using include Mongoid::Geospatial doesn't work :( You see any way around this?

Please make a test for this scenario and see if you can find a nice workaround. Thanks!

module GeoLocatable
  extend ActiveSupport::Concern

  included do
    include_shared :addressable

    include Mongoid::Geospatial
    include Geocoder::Model::Mongoid

    Point = Mongoid::Geospatial::Point

    after_create :perform_geocoding

    # rake db:mongoid:create_indexes
    field :location,  type: Point          

    spatial_index :location

    geocoded_by :address_str, coordinates: :location, skip_index: true
  end
Collaborator

nofxx commented Sep 22, 2012

What if we made geocode an option in mongoid_geospatial? I mean, we will not geocode it, there are 300 gems for it (which are you using, btw?) but just be aware of it... the point object.

I will make a small gem for this anyways - mongoid-indexes. I'm using geocoder gem. Yes, geocode could be an options somehow. I'm using geocoder with mongoid_geospatial just fine, but had some trouble generating good test data...

niedhui commented Nov 6, 2012

I'm used ryanong/mongoid_spacial.git before, and the geo_near is very good ,it use geoNear command, and return the distance calculated by mongodb, will this featured being considered?

Hmm, looking at my code example before. I see no reason why you would want to define first the Point constant, then the geo field and then the index. They all "belong together" as I see it. Just fluff...

Why not package this pattern into something like the following:

geo_field :location

Which would add the :location field of type Point and make a spatial_index for it as well (perhaps with special option to disable indexing)

geo_field :location, index: false

I'm not sure why the geoNear command from the original mongoid_spatial was removed in favor of calculations by RGeo etc. I would think that using the values from the database would be way more efficient (better performance).
Perhaps this functionality should reside (could be extracted into) in yet another gem? Hmm...

I'm introducing this little convenient class level macro ;)

geo_field :location, :spatial => true

Collaborator

nofxx commented Nov 13, 2012

@niedhui, @kristianmandrup geoNear is not implemented in Origin. http://mongoid.org/en/origin/docs/selection.html

@kristianmandrup checking out the merge, thanks in advance.

niedhui commented Nov 14, 2012

@nofxx yeah, we may need mongo_session#command if want this feature

Hmm, could we somehow combine this gem with mongoid_spatial in an elegant fashion. Perhaps create some kind of optional wrapper for it? Or at least include some documentation on how to use them together?

or some specs... or even a sample dummy app that integrates this gem with geocoder, georuby, mongoid_spatial and GoogleMapsForRails ;)

Exoth commented Dec 4, 2012

Inside my project I added a new class Coordinates, which inherits from Point and just adds all the methods like lat and lng as aliases. Then added a Mongoid field option delegate_coordinates, which delegates all these methods and also can validate latitude and longitude. If these features are needed, I'll make a pull request. What do you think?

Sounds awesome @Exoth :) Good idea! Please make the pull request so we can take a look. Cheers!

Collaborator

nofxx commented Jul 22, 2015

@Exoth Totally missed you comment man, sorry! Gonna check!

UPDATE: There's already the delegate helper. With it, instead of: obj.geom.x you call obj.x.
If we add a helper to validate coordinates (in a globe) == the same result you propose?

Collaborator

nofxx commented Jul 22, 2015

Helper for leaflet?
One thing I'm doing over and over is:

{ t: thing.name, m: thing.geom.to_a.reverse }
  • A lil helper for Point: MongoDB uses X, Y, leaflet,google uses Y, X.
  • Document: Build that hash for maps (t for label, m for point iirc that's leaflet convention)

@kristianmandrup thoughts?

@nofxx nofxx modified the milestone: 5.0.0, 3.0.0 Jul 22, 2015

nofxx self-assigned this Jul 22, 2015

nofxx added the enhancement label Jul 22, 2015

List 3 ways you think it could be beat done, most convenient while maintaining flexibility. Then we decide ;)

Collaborator

nofxx commented Jul 23, 2015

Added Point#reverse. So you dont need .to_a.reverse

Awesome!

Exoth commented Jul 23, 2015

Sorry guys that I haven't managed to do the pull request in these like 3 years. Thanks for bringing it up.

Collaborator

nofxx commented Jul 24, 2015

@Exoth was thinking in your idea, what do you think:

  field  :foo,  type: Point

  validates :foo, geospatial: { earth: true  OR polygon: true OR geometry: true  }

earth -> (need better name) is it spherical, (-180..180)
polygon -> it is closed
geometry -> only positive values valid +

Collaborator

nofxx commented Jul 24, 2015

earth could be used with polygon*
geometry could be used with polygon*
geometry can't be used with earth

Exoth commented Jul 24, 2015

I guess that would be useful.

@dblock dblock added question and removed enhancement labels Nov 21, 2016

Startouf commented Jan 2, 2017

Following @kristianmandrup remark on the Point namespace, until something is done I suggested the changes in the readme in #53

I also think there should be better support for using lat/lng (instead of x and y or arrays) to avoid confusions with other libraries such as Google that use LatLng (cf my comment in the same PR)

For instance, I have monkeypatched the gem in an initializer to add the following methods. I would love to be able to call model.coordinates.lat or even model.lat (I often confuse which one of latitude/longitude is the horizontal/vertical direction anyways, so x/y doesn't help me much ^^")

module Mongoid
  module Geospatial
    class Point
      alias :lng :x
      alias :longitude :x
      alias :lat :y
      alias :latitude :y

      def to_latlng_a
        [lat, lng]
      end
    end
  end
end
Collaborator

nofxx commented Jan 4, 2017

That's great. One thing, let's drop the _a? #to_latlng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment