[Code health] Harmonize geometry representation

[gino-m]

Shapes are represented in remote db in features as point (GeoPoint), geoJson (string) and geometry (map). There's also some confusion in the implementation about geometries vs LOIs. This design wasn't intentional and we can improve it.

Ideas:

• [x] Define generic geometry types independent of Ground model (perhaps import ~GeoTools?~ JTS)
• [x] Instead of extending LocationOfInterest, have it contain an arbitrary Geometry
• [x] Replace separate point, geoJson db format mirror GeoJSON where appropriate, but use Firebase GeoPoint for coordinates. Avoid nested arrays since they're unsupported by Firebase.
• [x] Load/save in local db (@scolsen)
• [x] Update map to render new Geometry types (@jsunde)
• [x] Delete old subclasses of LocationOfInterest (@scolsen)

[scolsen]

Am I right in thinking "Moving all to a map" means we'll unify them all under Geometry?

Point : Geometry
Polygon : Geometry
GeoJson : Geometry

[gino-m]

Am I right in thinking "Moving all to a map" means we'll unify them all under Geometry?

Point : Geometry
Polygon : Geometry
GeoJson : Geometry

Sorry, our messages crossed each other; I removed the bit about Map and tried to add some more clarity. PTAL!

[scolsen]

Instead of extending LocationOfInterest, have it contain an arbitrary Geometry

This is a definite yes.

Define generic geometry types independent of Ground model (perhaps import GeoTools?)

I think we should probably do this too, but I'm not sure if we should use GeoTools or not—I'd like to refactor LOIs first sticking with our own representation then figure it out from there.

[gino-m]

Define generic geometry types independent of Ground model (perhaps import GeoTools?)

I think we should probably do this too, but I'm not sure if we should use GeoTools or not—I'd like to refactor LOIs first sticking with our own representation then figure it out from there.

Good idea. Not sure either, but I see GeoTools has a permission license (LGPL), and includes [de]serialization of their model to/from GeoJSON.

[scolsen]

Yeah, I looked at it a bit more and it seems like its very robust. At the moment, I'm just trying to get gradle set up to try it out.

[gino-m]

We spoke. I'll summarize here:

• [ ] Define model classes and GeoJSON (de)serialization (import GeoTools?) @scolsen
• [ ] (De)serialize geometries as GeoJSON in local db @scolsen
• [ ] Define MapFeature, which contains tag: T, geometry: Geometry, and style: Style. Update GoogleMapFragment to use these. @gino-m
• [ ] Encode geometries in Firestore with two special rules @gino-m
  • Coordinates are stored as Firestore GeoPoint objects
  • All arrays are converted to indexed dictionaries i.e. {0: a, 1: b, 2: c,...}

@google/ground-maintainers FYI.

[gino-m]

@scolsen Before I forget - do you think we should support Polygon as well? The polygon drawing tool will only be capable of drawing a single polygon, but we could just represent that as a MultiPolygon with one element. Not sure what's better here. Any thoughts?

[scolsen]

@scolsen Before I forget - do you think we should support Polygon as well? The polygon drawing tool will only be capable of drawing a single polygon, but we could just represent that as a MultiPolygon with one element. Not sure what's better here. Any thoughts?

I think it comes down to whether or not we want to support re-editing. If we cast them to multipolygons users won't be able to modify the shape of the polygon or re-draw it after save—if we only use muiltipolygons, there's no way we'd be able to tell which one should be editable—that said, we could just generally let them edit any "shell" of a multipolygon (afaict, it's not possible to go from multipolgon -> List)

[gino-m]

Iiuc, MultiPolygons with only one Polygon are equivalent to a single Polygon. We could always check whether there's only one to allow editing.

[gino-m]

Hi @scolsen! What else is left? Are rendering, local persistence, and editing all moved over to the new model? If not maybe we should split this into separate issues to make them easier to assign?

[JSunde]

Oh I think there's more to this bug than I realized, I just saw the "TODO(#929)" and marked as fixed after my change, but I think there is still more to be done.

[gino-m]

There are three steps left, all assigned to @scolsen:

• Load/save in local db
• Update map to render new Geometry types
• Delete old subclasses of LocationOfInterest

@scolsen @JSunde @shobhitagarwal1612 Would you be able to collaborate on these so feature work on the map can be unblocked?

[JSunde]

Yea I think that sounds good.

Right now the GoogleMapsFragment should be able to render Points, Polygons, and MultiPolygons. I think we still need to add support for:

• [ ] LinearRing
• [ ] LinearString

Also I think we need to update the MapContainerViewModel and PolygonDrawingViewModel to create LOIs containing those geometries, instead of MapPins and MapPolygons.

One question, @gino-m do we currently have anything that we were previously rendering that is equivalent to the LinearRing and LinearString geometries?

[gino-m]

LinearRing is basically a polygon w/no holes. I don't think we need to render those. LinearString was rendered as part of the polygon drawing feature (for the an unfinished polygon being drawn)

[scolsen]

LinearRing is basically a polygon w/no holes. I don't think we need to render those. LinearString was rendered as part of the polygon drawing feature (for the an unfinished polygon being drawn)

ah, that's right, we called it PolyLine before i think

• [ ] Add constraints to each geometric object (e.g. it should not be possible to construct a linear ring that is not an actual ring (closed)).
• [x] Render line strings in the polygon drawing view; and we should be able to:
  • detect when these are closed and transform them into a LinearRing
  • Autocomplete a LinearRing from line strings.

[JSunde]

I just merged the updates to the PolygonDrawingViewModel, #1279, to remove the MapPolygon and render a LineString for incomplete polygons and convert them to a LinearRing when they are completed.

I think that may have been the last of the rendering changes required.

Let me know if you think there is anything I missed! Otherwise I think we can probably close this issue.

[scolsen]

I think we can consider this one closed!

[gino-m]

Whoohoo! Great work!!!