OpenGeoscience / uvdat

UVDAT Urban Visualization and Data Analysis Toolkit
Apache License 2.0
2 stars 1 forks source link

Add DerivedRegions from Region Union/Intersection #19

Closed jjnesbitt closed 1 year ago

jjnesbitt commented 1 year ago

Closes #14

This PR adds derived region creation functionality, currently only allowing individual region union as a way to create these derived regions. More functionality will be implemented as part of #25.

This PR is rather bulky, as it involves a large re-work to the web app. Ideally this rework wouldn't be bundled into this PR, but it was done in the pursuit of supporting Derived Regions. The plan is to merge this PR with as little additional changes as possible, now that the bulk of the re-work is done, and derived regions are supported in some fashion. Further iteration can be done in follow up PRs.

jjnesbitt commented 1 year ago

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

annehaley commented 1 year ago

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

Yeah, I like that organization a lot more. However I do notice that the createVectorLayer function is only used for derived regions, and every Dataset with geodata_file defined uses createVectorTileLayer. There should be a separation between vector datasets that use tiles and vector datasets that don't. So if a Dataset has vector_tiles_file populated, it would use the tiled source. If it doesn't have a vector_tiles_file but does have a geodata_file, then a normal vector source should be used.

Unless, of course, these changes depend on your vector tile optimization. In that case, would every vector dataset use tiling?

jjnesbitt commented 1 year ago

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

Yeah, I like that organization a lot more. However I do notice that the createVectorLayer function is only used for derived regions, and every Dataset with geodata_file defined uses createVectorTileLayer. There should be a separation between vector datasets that use tiles and vector datasets that don't. So if a Dataset has vector_tiles_file populated, it would use the tiled source. If it doesn't have a vector_tiles_file but does have a geodata_file, then a normal vector source should be used.

Unless, of course, these changes depend on your vector tile optimization. In that case, would every vector dataset use tiling?

I see what you mean. I do think they should be handled differently, but I'm confused on how it would work in this scenario. For a dataset that does not have vector_tiles_file defined, but does have geodata_file defined, and isn't a region dataset, what source would be used for the vector layer? Currently, the only way a vector layer is created from a dataset is through regions.

If we don't have a clear answer to the above question, I think it might be okay to just wait until the vector tiles optimization is merged in. In that case, derived regions may be the only "user" of vector layers, since we could just use vector tiles for everything else. We could even use vector tiles for derived regions as well, but that might be overkill.

What do you think?

jjnesbitt commented 1 year ago

Also, I'll work to resolve the current conflicts since #20 is now merged.

annehaley commented 1 year ago

I see what you mean. I do think they should be handled differently, but I'm confused on how it would work in this scenario. For a dataset that does not have vector_tiles_file defined, but does have geodata_file defined, and isn't a region dataset, what source would be used for the vector layer? Currently, the only way a vector layer is created from a dataset is through regions.

If we don't have a clear answer to the above question, I think it might be okay to just wait until the vector tiles optimization is merged in. In that case, derived regions may be the only "user" of vector layers, since we could just use vector tiles for everything else. We could even use vector tiles for derived regions as well, but that might be overkill.

What do you think?

For Datasets, vector_tiles_file should be used as the input for a VectorTileSource if it exists. Else if geodata_file exists, it should be used as the input for a VectorSource. The contents of geodata_file are a geojson that can be used as vector data.

I definitely want to get your tiling optimization involved, but this doesn't need to depend on it. I still think the size of the geojson data should be our threshold for determining whether vector data should be tiled (whether that be a vector Dataset, Region, or DerivedRegion, though the latter two are not likely to exceed a size threshold). That way we don't end up creating too many VectorTile objects for vector data that doesn't need to be tiled.

jjnesbitt commented 1 year ago

For Datasets, vector_tiles_file should be used as the input for a VectorTileSource if it exists. Else if geodata_file exists, it should be used as the input for a VectorSource. The contents of geodata_file are a geojson that can be used as vector data.

This has been addressed in 58d9481

jjnesbitt commented 1 year ago

I still think the size of the geojson data should be our threshold for determining whether vector data should be tiled (whether that be a vector Dataset, Region, or DerivedRegion, though the latter two are not likely to exceed a size threshold). That way we don't end up creating too many VectorTile objects for vector data that doesn't need to be tiled.

I think that makes sense, although I'll point out that I think the current threshold for this of 100mb is far too high, as I see lag when rendering datasets even in the single MB range of sizes. So realistically the range of sizes that we can afford not to tile with vector tiles may end up being small, but nonetheless I agree.

Regarding creating too many VectorTile objects, I don't think that's a real concern right now. We would have to expand much beyond the scope of the current project to create enough of these tiles to see performance issues, and even if it came to that, indexing / optimization should get us very far in that regard.

annehaley commented 1 year ago

@AlmightyYakob I tried out the new Active Layers overlay and thought it could use a couple things:

I made these changes in 2e0b8d4, let me know what you think. Otherwise, I really like how this component looks, and I think it is a better place for this information.

jjnesbitt commented 1 year ago

@AlmightyYakob I tried out the new Active Layers overlay and thought it could use a couple things:

  • Tooltips on the icons so the user can understand their functions better (especially important for a button that clears all layers)
  • Text to fill the component when there are no active layers to show
  • Some spacing adjustments

I made these changes in 2e0b8d4, let me know what you think. Otherwise, I really like how this component looks, and I think it is a better place for this information.

Just checked it out, looks great! Thanks for those changes.