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

[WIP] point-clouds: generate virtual point cloud (.vpc) #955

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rcoup
Copy link
Member

@rcoup rcoup commented Dec 5, 2023

Proof of concept generating a Virtual Point Cloud (VPC) file for datasets based on tiles present in the current working copy. The VPC can be opened in eg: QGIS rather than loading individual tiles, and works similarly to VRT files for rasters.

  1. Currently uses pdal_wrench to generate the files, and we're not building/distributing that, needs to be in the PATH.
  2. You also need to set KART_POINT_CLOUD_VPCS=1

Still need to decide either to bundle pdal_wrench, or write minimal VPC files ourselves.

Related links:

Checklist:

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

Based on the files present in the current working copy. The VPC can
be opened in eg: QGIS rather than loading individual tiles, and works
similarly to VRT files for rasters.
@hamishcampbell
Copy link
Member

At some point we expect to support generating STAC indexes for datasets in Kart. Given VPCs are based on STAC, and the relative simplicity of the format, maybe we should generate them ourselves.

@olsen232
Copy link
Collaborator

olsen232 commented Dec 5, 2023

At some point we expect to support generating STAC indexes for datasets in Kart. Given VPCs are based on STAC, and the relative simplicity of the format, maybe we should generate them ourselves.

I was going to say the opposite - let's just bundle pdal-wrench, seems simpler (unless we hit some difficulty with that approach).
We can always switch approach later if none of the tools we already bundle / are okay to bundle do everything we want them to.

Copy link
Collaborator

@olsen232 olsen232 left a comment

Choose a reason for hiding this comment

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

Two things left to do

  • make this actually work by bundling pdal_wrench, or some other solution. We can still leave it turned off by default if we're not confident about it yet, but better to have a feature you can turn on and test rather than something where we have to install it to test it and then who knows if it's working the same for everyone (like we had with PDAL for a while)
  • add a test - there's two very basic tests for VRTs:
    test_working_copy_vrt, test_working_copy_vrt_disabled

vrt_path.unlink(missing_ok=True)
return

with NamedTemporaryFile("w+t", suffix=".kart_tiles", encoding="utf-8") as tile_list:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haven't tested it, but whenever I see this kind of thing I think "this will probably break on windows..." but it depends, windows can certainly share an open file between two processes if the processes feel like it, but seems like sometimes they don't.
Anyway if it does break on windows, this will be why.

@hamishcampbell
Copy link
Member

Two things left to do

* make this actually work by bundling pdal_wrench, or some other solution. We can still leave it turned off by default if we're not confident about it yet, but better to have a feature you can turn on and test rather than something where we have to install it to test it and then who knows if it's working the same for everyone (like we had with PDAL for a while)

* add a test - there's two very basic tests for VRTs:
  `test_working_copy_vrt`, `test_working_copy_vrt_disabled`

Ok, if bundling is straight forward that seems like a sensible step. It has a few other useful functions we might want in future too.

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