acteng / atip

Active Travel Infrastructure Platform
https://acteng.github.io/atip/
Apache License 2.0
22 stars 4 forks source link

Superschemes for sketcher #379

Closed Pete-Y-CS closed 10 months ago

Pete-Y-CS commented 10 months ago

Alright this is still very rough and ready: both the code, and what we're calling stuff will want some tweaking but we have here a first pass at having subschemes to which interventions belong. https://acteng.github.io/atip/superschemes-for-sketcher/scheme.html?schema=pipeline&authority=TA_Lincolnshire&style=streets#9.32/53.1242/-0.4517

Currently you should be able to:

dabreegster commented 10 months ago

I realize we haven't written down a design doc or anything to describe the purpose of this idea, so I want to start with that. I'll review what this PR does next.

Use cases

1) Short-term: LCWIP PDFs tend to show many schemes on one map. People sketching them likely want to do the same -- so this is a way to edit multiple schemes at the same time. 2) Speculative long-term: When some future funding round starts, if an authority already has a bunch of planned schemes mapped out, they could load their one or more LCWIP schemes, then extract/select/copy some things into a new scheme for submission.

Data modelling questions

Should a feature belong to exactly one scheme, or can it belong to multiple at a time? #372 brings up some ambiguous cases:

As soon as multiple schemes share the same feature, we have to decide how that should work -- are there two copies of something that can diverge, or do they reference the same single object? My inclination is towards the latter -- otherwise we have to communicate through the UI something quite confusing.

A minimal design

152 was an earlier attempt at this, but here's a fresh idea. The simplest UX I can think of to satisfy the two use cases would look like this:

The GeoJSON representation would pretty much match what's used for all_scheme_data in the browse page right now.

Implementing this should be pretty straightforward, except for the problem of how to save files. Right now one exported GJ file (and equivalently, one stringified GJ object in local storage) represents one scheme, and is named by authority and scheme name. We could save multiple files / local storage keys, one per scheme? Or maybe we should natively handle a file that has everything bundled together with this extra schemes object, and in simple cases, just have one scheme. Both approaches are perfectly compatible with properly storing data in a DB through a future API call.

Pete-Y-CS commented 10 months ago

Re: the bug you mentioned. I wasn't seeing that locally. I will try to work out if I failed to push some key change or what.

Re sub/super schemes the naming got away from me a bit, because I wanted to use existing infrastructure for populating 'schemes' but my idea is that the subschemes are effectively schemes. They will have more data collected about them and this is the use case we are trying to deal with: an LCWIP is a superscheme, and then the (sub)schemes are meaningfully distinct: they will have (some of the following:) names, routes with segments, a priority level (low, medium, high).

Ultimately it's editing multiple schemes at the same time, I agree. I think what I've done is already equivalent to all_schemes_data.geojson, no?

dabreegster commented 10 months ago

an LCWIP is a superscheme, and then the (sub)schemes are meaningfully distinct: they will have (some of the following:) names, routes with segments, a priority level (low, medium, high).

I'm confused by this... based on the latest data spec doc, #380 adds a bunch of fields at the scheme/LCWIP level, and changes the per-feature ones. I don't think priority is in there?

If our goal is editing multiple schemes at a time without trying to establish relationships between schemes, I think there's a simpler design possible

Pete-Y-CS commented 10 months ago

Timescale is synonymous with priority here. Timescale was what Robin and I spitballed early on, I've hear Guy use priority since. In any case if there's a simpler solution I'd be down, although I think it's going to end up similar data wise to what I've already done? Might be another one for a call to work out where we're differing, but I think that PR you linked looks great abut could presumably be plugged into something like what I've done here (bug of interventions not showing notwithstanding)

dabreegster commented 10 months ago

About to look at this again, but just to note, it's failing tests / TS checks currently

Pete-Y-CS commented 10 months ago

About to look at this again, but just to note, it's failing tests / TS checks currently

Seems to pass locally, might be one of the flakey tests

dabreegster commented 10 months ago

npm run check breaks for me locally too https://github.com/acteng/atip/actions/runs/6708978389/job/18258013700

dabreegster commented 10 months ago

I need to figure out how to repro this, but I lost everything on the map after making something: Screenshot from 2023-11-01 10-41-09

Pete-Y-CS commented 10 months ago

Yep I ran npm run test not check. Solved most except an issue with turf bbox is puzzling because it was simply me moving around some code which preexisted

dabreegster commented 10 months ago

Another bug: if I delete the default subscheme and wind up with none, then creating something new breaks:

Screenshot from 2023-11-01 10-43-24

There are enough big structural changes here that I think some Playwright tests would be helpful. How about:

Pete-Y-CS commented 10 months ago

Good idea, I'll get on em, type checks resolved

dabreegster commented 10 months ago

I'm happy to merge this into main and iterate. Some thoughts on follow-ups: