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

remove extents VLR and add gpstime_minimum and gpstime_maximum to info VLR #50

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

hobu
Copy link
Contributor

@hobu hobu commented Oct 26, 2021

#47 suggests removing the extents VLR entirely. This approach does so in the following way:

  • Add gpstime_minimum and gpstime_maximum to the info VLR
  • Remove extents VLR

The rationale for this is an implementor can infer extents information for every other dimension type except GPSTime.

@timodwhit
Copy link
Contributor

Is there an idea to add the stats VLR with this change?

@hobu
Copy link
Contributor Author

hobu commented Oct 26, 2021

Is there an idea to add the stats VLR with this change?

I don't know. GPSTime is the only continuous variable a reader wouldn't have the ranges for. Extra Bytes dimensions should have that info written into the VLR. Returns and XYZ extents are in the header. Intensity and others like PointSourceId are bounded by data type. Minus GPSTime, the value of extents is quite minimal, but it adds complexity to the writers.

As for the stats VLR, it all seems to be simply too reader-specific and the more complex we make it, the harder we make writer implementations for value that is quite poorly distributed across consumers of the data. Very special and specific renderers might get a big boost, others are likely not to care, and analysis-oriented workflows might never touch it at all.

A specialized reader can accumulate stats in whatever form it desires and append it to the COPC file as an EVLR in an unobtrusive way. Having the octree structure of COPC might even make that task even easier to achieve. I think this is what desktop renderers should do, given we have people asking for per-node statistics to support advanced filtering for analysis.

Browser-based viz is a special use case that can't conveniently do this though. I'm hoping having GPSTime's ranges is enough.

@timodwhit
Copy link
Contributor

That does make sense and I definitely can see the appeal of reducing the complexity and/or duplication of data.

I do wonder about the DX of the extents vs the DX of removing extents. If extents is in a single object (even if duplicated) it is potentially easier to access the extents and to make a single request for those (in the case of a browser).

Without the extents, you have to keep in mind where all the ranges can be found and how they can be parsed instead of a single object.

@hobu
Copy link
Contributor Author

hobu commented Oct 26, 2021

Without the extents, you have to keep in mind where all the ranges can be found and how they can be parsed instead of a single object.

I agree. LAS is annoying in this way, but it's too late to fix it.

@timodwhit
Copy link
Contributor

Sorry, not following why we can't add/keep the VLR for that with this format?

@CCInc
Copy link
Contributor

CCInc commented Oct 26, 2021

This may be a good compromise if you don't want to keep the entire extents vlr. The only other value I can think of that would benefit from a min/max field is Intensity, because the range of intensity often doesn't fill the entire byte range.

I would not mind having an optional stats vlr to have a consistent format for these additional dimensions, to have some dimension of interoperability. Then, we can get all the info we need for values like intensity, and put histograms for classifications or whatever else. Or if you don't want to go that route, I may just keep the extents VLR on my writer out-of-spec. I think for the base spec, however, having gps-time is sufficient and reasonable.

I think this is a good start though, given the specification complexities of the extents 👍

@abellgithub
Copy link
Collaborator

If the writer of a file has not properly scaled intensity to meet the LAS specification, then they have made an error.
From the spec:

Intensity, when included, is always normalized to a 16 bit, unsigned value by multiplying the value
by 65,536/(intensity dynamic range of the sensor).

@CCInc
Copy link
Contributor

CCInc commented Oct 26, 2021

That is certainly what should happen, however in the real world it hardly ever does. In fact, your own autzen stadium file does not conform:

image

@hobu
Copy link
Contributor Author

hobu commented Oct 26, 2021

your own autzen stadium file does not conform

The Autzen file is from 2010 originally delivered as a 1.0 LAS file IIRC. Maybe we should fix that.

@CCInc
Copy link
Contributor

CCInc commented Oct 26, 2021

funny enough, I have also seen two public datasets today which use RGB range 0-255 instead of 0-65525:

  Color R 2 255
        G 3 255
        B 4 255

Not that this is necessarily a copc problem, but having the rgb max stored would provide some info on whether that's the case

@CCInc
Copy link
Contributor

CCInc commented Oct 28, 2021

Hey! Just to follow up on this - we decided to roll our own optional extended stats VLR providing mean/variance, following the same format as the existing extents VLR. If this PR gets merged, we will be happy to bring the extents VLR under the rock_robotic userid, and we'll update copclib to add gpstime min/max to copcinfo as proposed. If a stats VLR gets added to the spec in the future, we'll update copclib to reflect that. Let me know your thoughts.

I think this PR can clear up confusion regarding extents, and providing gps-time in the header will satisfy most use-cases.

@hobu
Copy link
Contributor Author

hobu commented Nov 9, 2021

After collating feedback from other implementers in both the analysis- and visualization- oriented software domains, there was general agreement that having the gpstime extents in the info VLR (something missing in LAS) is enough, and the utility of the extents VLR by itself without out other more useful statistics is very limited.

Merging this and we will work to update the example files in the next few days.

@hobu hobu merged commit 703c51e into main Nov 11, 2021
@hobu hobu deleted the extent-removal branch November 11, 2021 15:27
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.

4 participants