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

feat: added format handler for different geometry types for jsonform #1276

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

santilland
Copy link
Member

@santilland santilland commented Sep 26, 2024

Implemented changes

Screenshots/Videos

Checklist before requesting a review

Copy link

netlify bot commented Sep 26, 2024

Deploy Preview for eoxelements ready!

Name Link
🔨 Latest commit c6abbd9
🔍 Latest deploy log https://app.netlify.com/sites/eoxelements/deploys/674d871785e03d0008ce5317
😎 Deploy Preview https://deploy-preview-1276--eoxelements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@santilland santilland marked this pull request as ready for review November 8, 2024 14:18
@santilland santilland changed the title feat: added initial bounding box format handler for jsonform feat: added format handler for different geometry types for jsonform Nov 8, 2024
@santilland
Copy link
Member Author

santilland commented Nov 8, 2024

@silvester-pari i think this brings great first implementation to support different types of geometries into the jsonform using the draw tools. There will be nuances that need to be solved, e.g. also to better support specialized STAC schemas, but i think as is it already provides great value and would propose to merge and continue improving in smaller increments afterwards.
Thank you @A-Behairi for the implementation!

Copy link
Collaborator

@silvester-pari silvester-pari left a comment

Choose a reason for hiding this comment

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

Thank you, looks like a big addition! I am unsure about a few points however:

  • the *-editor formats: I believe it doesn't make sense to offer this distinction, as the drawing of a bbox should always allow to edit the coordinates manually (via a number input) in order to fine-tune the drawing. I get that the current editor that comes with drawtools is not ideal for that (layout, and showing the entire feature content instead of just the coordinates), but here we maybe can think of a different solution (e.g. keep the coordinate input fields as per default json-editor, and just add the map additionally)
  • having to click the "draw" button in order to be able to start drawing: I feel like from a user perspective this is an unnecessary step and could cause quite some friction. I understand that this is the technical solution we have, but it's less than ideal from the user experience. Ideally the user comes to the section where the map appears (by default it should not be scroll-zoomable by the way) and can simply draw the corresponding shape
  • changing of the current example schemas: it's probably not wise to change the current story schemas (as in the collection schema) just to showcase something that works quite differently; the current example schema is designed with STAC in mind and I wouldn't like to completely dismiss this (when we remove it now from the codebase, will it ever come back?). I would have hoped that the new functionality would be an addition that works with the existing schema, and not require a completely new one.

All in all, it feels a bit like a very much technology-driven solution, not necessarily a developer- and user-friendly one; maybe we can find compromises, offering less functionality but in a more user-friendly manner? 🤔
Maybe also the drawtools are not the ideal fit for the jsonform, and we maybe should just use the map and then some input fields generated by jsonform; but happy to do some more brainstorming and/or experimentation!

@A-Behairi
Copy link
Collaborator

Thank you for your feedback! I would like to note that the implementation was intended with much developer experience and user experience in mind, I would like to clear and comment on some points:

  • the implementation doesn’t change or interrupt any of the current supported schemas/editors, it builds on top of it by introducing type: “spatial” with various supported formats, options, and a custom validator.

(when we remove it now from the codebase, will it ever come back?)

That being said, we can simply revert the change in the collection and it would go back to how it was.

  • I can see that there’s alot of enhancements made to the current collection schema vs the actual stac collection spec prior to this PR (for example the license property). The changes that this PR introduced doesn’t misregard the STAC collection spec at all, the returned value from { type:”object”, format:”bounding-boxes }” is an array of 4 element number arrays, which is matched by the spec.



PS. the changes has been made after @silvester-pari asked in the daily about how would this PR be integrated with the collection schema, and it was intended to show case that.


  • the spatial editor has a title property that can guide the user to select/draw an area/feature on the map, please checkout the the image below as an example :
Screenshot 2024-11-15 at 11 01 23

I think this fulfills the requirement of it being UX friendly, but If you think that it would be better to start the drawing without pressing on the button, I would be happy to implement that.

  • regarding the *-editor formats, I think this is intended for advanced users, I agree it could look cleaner and I think we can either include it now and iterate on it in the future or remove it and implement it as a separate feature in a more UI/UX friendly way.

@silvester-pari
Copy link
Collaborator

Thank you @A-Behairi for the detailed answer! Here are my thoughts:

  • *-editor formats, I agree with your suggestion to remove them completely for now and work on this in a future PR
  • drawing without pressing button: how about we do a compromise, and say if it uses an external map (for property) then keep as it is, if it does not use for then enable drawing automatically?
  • type spatial: I will try to explain better what I mean below:

This was the previous schema:

              "properties": {
                "bbox": {
                  "type": "array",
                  "items": {
                    "type": "array",
                    "format": "bounding-box",
                    "items": { "type": "number" },
                    "minItems": 4,
                    "maxItems": 4
                  },
                  "minItems": 1
                }
              }

For bounding boxes, I would imagine something like this:

              "properties": {
                "bbox": {
                  "type": "array",
                  "format": "bounding-boxes",
                  "items": {
                    "type": "array",
                    "format": "bounding-box",
                    "items": { "type": "number" },
                    "minItems": 4,
                    "maxItems": 4
                  },
                  "minItems": 1
                }
              }

Note a few things:

  • on the "parent" level, the type is array, since it is expected to output an array. Only the format is bounding-boxes, which renders a map with multi-bbox input instead of an array editor
  • on the "child level", the type is still array as it expects an array of coordinates as output, and the format is bounding-box, since it allows drawing a single bbox on a map
  • if for example the "parent" would have no format specified (only the children), it would render an array editor, and each array would have its own bounding-box map. By providing bounding-boxes format on the parent, it allows combining all these in one map instead of having four maps
  • there could be usecases where only a single bounding-box is needed
  • the type spatial is not really describing an output format, so I'm not sure we need to have a type spatial. I hope it works to just implement a custom rendered for the type array

@santilland
Copy link
Member Author

santilland commented Nov 21, 2024

hi! it seems there is quite some definition work that was needed for this, as it is bringing in concepts needed in multiple places, i will try to recap things to see if we agree on it (i already had a quick catch up with Ahmed about these points).

  1. we agree to remove the *-editor cases. As i see it in the future we will want to have a dedicated field for the values, allowing also to edit them, some thoughts on this later
  2. as for automatic drawing activation i think changing the behavior depending on the "scenario" can be very confusing for someone using the eox-json-form. My proposal would be we set the automatic start of selection handler as default and provide an option to disable starting the selection handler (e.g. autoStartSelection: false)
  3. changing the type to array makes sense for bbox and polygon but for feature it gets more complicated. More on that below.

Topic:

  • fields for editing values: the default way to handle arrays in json form is to have dedicated fields for each value, this should be fine for bbox as it is only 4 values, for a polygon though it would be a crazy number of fields. I also think there are a number of scenarios where a user selecting on the map is only interested in the "general" area, and not interested in concrete values, so we should also build in the option to disable showing the editing fields (once this is implemented)
  • changing type of feature(s): the idea with feature(s) is allowing selecting a vector feature and extracting a property of it, this can be defined in the options. This property could be many things, e.g. it's extent, identifier, name, geometry, .... I think we need to explore if we can use a feature format for a generic type, that might be one possible approach

Additionally:
After talking use cases with Elias i realized we need also point selection. I saw draw tools supports this, so i would propose to call the format point? Would also be of type array, should be very aligned to bbox 2 values instead of 4.

@santilland
Copy link
Member Author

Trying to integrate a new statistical api experiment i realized that geometries can be expected in at least a couple of different ways.
In order to avoid needing strange conversion in the client i think we need to support these types, and thinking about how to solve this maybe we can nicely use the type to solve this.
Currently i see 3 different approaches:

  • geojson: geojson of the geometry
  • WKT: WKT representation of the geometry
  • array: array of the geometry (if multiselection allowed array of arrays)
    • point: 2 values
    • bbox: 4 values
    • polygon: N values

@A-Behairi
Copy link
Collaborator

Thank you for all your inputs, find below a list of all the adjusted schemas that renders the drawtools.

 // enables drawing multiple boxes
  // returns an array of extents
  {
    type: "array",
    format: "bounding-boxes",
  },
  // enables drawing multiple boxes
  // and returns a wkt formatted string
  {
    type: "wkt",
    format: "bounding-boxes",
  },
  // enables drawing multiple boxes
  // and returns a geojson object
  {
    type: "geojson",
    format: "bounding-boxes",
  },
  // enables drawing a single box
  // and returns the the extent of the bbox
  {
    type: "array",
    format: "bounding-box",
  },
  // enables drawing a single box
  // and returns a wkt formatted string
  {
    type: "wkt",
    format: "bounding-box",
  },
  // enables drawing a single box
  // and returns a geojson object
  {
    type: "geojson",
    format: "bounding-box",
  },
  // enables drawing multiple polygons
  // returns an array of the drawn features
  {
    type: "array",
    format: "polygons",
  },
  // enables drawing multiple polygons
  // returns a wkt formatted string
  {
    type: "wkt",
    format: "polygons",  
   },
  // enables drawing multiple polygons
  // returns a geojson object
  {
    type: "geojson",
    format: "polygons",
  },
  // enables drawing a single polygon
  // returns the drawn feature object
  {
    type: "object",
    format: "polygon",
  },
  // enables drawing a single polygon
  // returns a wkt formatted string
  {
    type: "wkt",
    format: "polygon",
  },
  // enables drawing a single polygon
  // returns a wkt formatted string
  {
    type: "geojson",
    format: "polygon",
  },
  // enables drawing multiple points
  // returns an array of the points coordinates
  {
    type: "array",
    format: "points",
      },
  // enables drawing multiple points
  // returns a wkt formatted string
  {
    type: "wkt",
    format: "points",
  },
  // enables drawing multiple points
  // returns as a geojson object
  {
    type: "geojson",
    format: "points",
  },
  // enables drawing a single point
  // returns the coordinates of the point
  {
    type: "array",
    format: "point",
  },
  // enables drawing a single point
  // returns a wkt
  {
    type: "wkt",
    format: "point",
  },
  // enables drawing a single point
  // returns a geojson object
  {
    type: "geojson",
    format: "point",
  },
  // enables selecting a feature from a layer
  // the returned type can vary based on the provided `type` and `options.featureProperty`
  // if no `optons.featureProperty` is provided the returned value is the feature object
  {
    format: "feature",
  },
  // enables selecting multiple features from a layer
  // the returned value is an array of values that can vary based on
  // `items.type` and `options.featureProperty`
  // if no options.featureProperty is provided the returned value is an array of feature objects
  // and the `items.type` should be set to "object"
  {
    type: "array",
    format: "features",
  },
  // enables selecting multiple features from a layer
  // the returned value is a wkt formatted string
  {
    type: "wkt",
    format: "features",
  },
  // enables selecting multiple features from a layer
  // the returned value is a geojson object
  {
    type: "geojson",
    format: "features",
  }

supported options:

  • autoStartSelection: if set to false the drawing doesn't start unless the user press on the icon.
  • projection: based on feat: enable configuring the projection of emitted features #1356 , returns the value in the specified projection
  • layerId: specific to formats feature & feature, selects a layer based on the id provided to select features from.
  • featureProperty: also specifc to formats feature and features, returns the value of the specified property of the feature.
  • for: string selector for the eox-map assigned to the drawtools, if not provided the editor creates a map and assign it to the drawtools.

@santilland
Copy link
Member Author

santilland commented Nov 29, 2024

Hello @A-Behairi this is looking great, the type options are really helpful to support different endpoint capabilities.
Trying to integrate one endpoint i realized that the geojson type is a little more complex as it allows many ways to structure it. I think we can in principle use the templating language to extract exactly what we need but it would be good to evaluate how we structure the geojson.
I would propose instead of using featurecollections for all, to use simple features for the single selection cases, i will try to add some examples below.
For the multiselect (points, bounding-boxes, polygons) it would make more sense to have feature collections, although again, reading about options
it could also be possible to use MultiPoint MultiLineString and MultiPolygon. I think i like this option more as it ensures all geometries are from the same type? In a feature collection there could potentially be any mix of things.
**edit: after some discussion in the forum, we think endpoints are less likely to support the Multi- variant type, and would expect probably a list of geometries instead, so keeping the featurecollection should simplify accessing multiple separate features, so i would propose to go that way for now.

Any opinion on this would be very welcome, please let me know what you think.

While writing this i realized i know of use cases where we will need to define a line, so if it is not too much effort we could use the opportunity to add this format also here, we could call it format: line and format:lines? I added an example also at the end.

Examples for single select:

{type: "geojson", format: "point"}

{
      "type": "Feature",
      "geometry": {
        "type": "Point",
        "coordinates": [
          11,
          43
        ]
      },
      "properties": {}
    }

{type: "geojson", format: "polygon"} and {type: "geojson", format: "bounding-box"}

{
      "type": "Feature",
      "properties": {},
      "geometry": {
        "coordinates": [
          [
            [
              11.186767387616754,
              44.019851231386326
            ],
            [
              11.230712935404142,
              42.17497898908604
            ],
            [
              12.428222447740097,
              41.9629345529863
            ],
            [
              13.988281347309453,
              42.734276566185855
            ],
            [
              13.39501999765912,
              44.2012799811516
            ],
            [
              11.186767387616754,
              44.019851231386326
            ]
          ]
        ],
        "type": "Polygon"
      }
    }

{type: "geojson", format: "line"}

{
      "type": "Feature",
      "properties": {},
      "geometry": {
        "coordinates": [
          [
            12.086527294554173,
            43.151923751754
          ],
          [
            13.318383515919493,
            43.141078166340606
          ],
          [
            13.790678332841026,
            43.17633857773657
          ]
        ],
        "type": "LineString"
      }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants