search

geotrellis / geotrellis-pointcloud

GeoTrellis PointCloud library to work with any pointcloud data on Spark
Apache License 2.0
26 stars 10 forks source link

Simplify EPTMetadata build function #61

Closed pomadchin closed 4 years ago

pomadchin commented 4 years ago

This PR address #60 and introduces a EPTMetadata function that does not read recursively all the entwine hierarchy files.

Fixes https://github.com/geotrellis/geotrellis-pointcloud/issues/60

jpolchlo commented 4 years ago

This change simply punts on calculating resolutions. Not great. We don't need that list to be accurate, but we should make sure that at least the base resolution (of the 0-0-0-0 node) is listed? We might request of the PDAL guys that ept.json indicate the max depth of the tree directly to avoid this problem in the future, but that change would be a long way off, and some may argue, not of huge value/interest to consumers of the tree.

pomadchin commented 4 years ago

@jpolchlo yes, it was in progress, check it out now.

jpolchlo commented 4 years ago

This is less than ideal. If we are going to provide a resolutions field, it should be accurate. I suggest that we just scrap this and only give the base cellsize. In this way we get the important information, without the false security that we somehow know the depth of the tree based on the number of elements in the resolutions list. Thus far, we haven't used the results of the resolutions list in the raster source, so this maybe isn't a big deal.

pomadchin commented 4 years ago

@jpolchlo reading this comment https://github.com/geotrellis/geotrellis-pointcloud/pull/61#issuecomment-604119910 I had a sense that we can still the old method of computing resolutions: This change simply punts on calculating resolutions. Not great. We don't need that list to be accurate.. In the next comment https://github.com/geotrellis/geotrellis-pointcloud/pull/61#issuecomment-604477806 it is mentioned that If we are going to provide a resolutions field, it should be accurate. What is the best way to interpret it?

I'm going to preserve the current resolutions computation method, since there are no other ways to get it at this point. I can add a comment that this is an approximate resolutions computation.

jpolchlo commented 4 years ago

Yes, I can see that those two comments seem to conflict, so a clarification: We are bound by interface to provide some kind of list of overviews, yes? I suppose we could list nothing at all and simply rely on the gridExtent to communicate the base resolution. What I don't think we should do is populate resolutions with an incomplete list. Showing nothing seems better. We can tune the internal logic for resolution selection when we actually build the query pipeline once the better OverviewStrategy implementation is available. That's where I was anticipating using the resolutions list, but we don't need it, since we know that the available resolutions follow a power-of-two progression.

pomadchin commented 4 years ago

@jpolchlo ok. I created a follow up issue as well https://github.com/geotrellis/geotrellis-pointcloud/issues/62

pomadchin commented 4 years ago

Going to merge it once Travis is happy and there are no other comments.

pomadchin commented 4 years ago

I created a separate issue https://github.com/locationtech/geotrellis/issues/3210

Mering this now, we'll change the behavior depending on the issue resolution.