Handle extended classifications. Issue #445

[arknoll]

No description provided.

[m-schuetz]

Hi, thanks for the PR! #336 seems to want to use the extended classification if the normal classification is 0. Do you know if one of these versions is correct or if it doesn't matter?

[rburgstaler]

I searched a bit to determine how exactly you determine whether to use point->extended_classification or to use point->classification to extract the classification as Martin would have intended. Maybe I missed it. I came up with the following samples that made me go down the path of using a zero value of point->classification to then go and try to use the point->extended_classification. The thought was that even if point->classification was zero and point->extended_classification did not exist, point->extended_classification would be 0 anyway so no biggie. @arknoll, I know you had questioned this in my #336 PR.

https://github.com/LAStools/LAStools/blob/master/LASzip/example/laszipdllexample.cpp#L2328
The following example says for point->classification that it must be set because it "fits" in 5 bits. Notice that the example also sets the point->extended_classification to the value that is less than 32 so the point->extended_classification ends up getting set anyway.

    point->intensity = 20;
    point->extended_return_number = 1;
    point->extended_number_of_returns = 2;
    point->classification = 5;                // it must be set because it "fits" in 5 bits
    point->extended_classification = 5;
    point->extended_scan_angle = 3700;
    point->extended_scanner_channel = 1;
    point->extended_classification_flags = 0; // no flag is not set
    point->gps_time = 53413162.580200;

The following example says for point->classification that it must be set to zero as the real value is stored in the extended field. Notice that the example also set the point->extended_classification.
https://github.com/LAStools/LAStools/blob/master/LASzip/example/laszipdllexample.cpp#L2328

    point->intensity = 70;
    point->extended_return_number = 9;
    point->extended_number_of_returns = 9;
    point->classification = 0;                // it must be set to zero as the real value is stored in the extended field
    point->extended_classification = 42;
    point->extended_scan_angle = 3633;
    point->extended_scanner_channel = 1;
    point->extended_classification_flags = 0; // no flag is not set
    point->gps_time = 53413162.566800;

I feel that ideally someone finds a link that documents or gives a code example of exactly how you determine which classification to use. I did see some places in the codebase that used check for extended_classification > 31. Is extended_classification gauranteed to have a value?
https://github.com/LAStools/LAStools/blob/150e6c23bb2ced93083b5ba656c5f8e55403b81b/LASlib/src/laswritercompatible.cpp#L352

   if (pointCompatibleDown.extended_classification > 31)
  {
    pointCompatibleDown.set_classification(0);
  }
  else
  {
    pointCompatibleDown.extended_classification = 0;
  }

I am kind of thinking that both ways of doing it would work. I am just not sure of how Martin intended it to work.

[pchilds]

@rburgstaler
In https://github.com/LAStools/LAStools/blob/master/LASzip/src/laspoint.hpp:
inline void set_extended_classification(U8 extended_classification) { this->extended_classification = extended_classification; if (extended_classification > 31) this->classification = 0; else this->classification = extended_classification; };

As such a reader for LasPoint can safely read with get_extended_classification if Point type >= 6 is in use. If you try to read with get_classification then you will get zeroed out fields for classifications >= 32

I think extended_classification will be uninitialised if you get a LasPoint via the copy constructor from a point with extended_point_type unset. Everything else seems to zero it first.

Your last example shows Martin zeroing out the extended_classification if it is less than 32. This was because prior to some 2017 commits LASzip wasn't propagating extended_classification to classification, and as such a reader would only expect one to be set.

I agree with you that both ways should work. Conceptually though I feel this PR does follow the logic of LASzip more closely. Myself I would have gone purely for an is_extended_point_type() check, but LASzip tends to not use this.

[pchilds]

I can confirm that this branch works well.

[m-schuetz]

thanks for the PR