ASPRSorg / LAS

LAS Specification
https://www.asprs.org/committee-general/laser-las-file-format-exchange-activities.html
155 stars 18 forks source link

Add compact ExtraByte VLR v2 #119

Closed esilvia closed 2 years ago

esilvia commented 2 years ago

The existing ExtraByte descriptor now has a great deal of deprecated and reserved fields, mostly as a result of the deprecation of the double/tuple data types in #1. Each descriptor requires 192 bytes, of which only 108 bytes or so are in use or reserved. We should release a compact ExtraByte Descriptor VLR (v2) that removes the deprecated values we no longer need.

We could also repurpose the first Reserved field as a Standard ExtraByte ID as described in #37.

Note that the new definition could also change the ExtraByte min/max storage to be a double to make it consistent with the LAS header's Min/Max XYZ, which we had attempted in #4 but couldn't in a LAS 1.4 Revision.

Existing definition:

struct EXTRA_BYTES {
    unsigned char           reserved[2];     // 2 bytes
    unsigned char           data_type;       // 1 byte
    unsigned char           options;         // 1 byte
    char                    name[32];        // 32 bytes
    unsigned char           unused[4];       // 4 bytes
    anytype                 no_data;         // 8 bytes
    unsigned char           deprecated1[16]; // 16 bytes
    anytype                 min;             // 8 bytes
    unsigned char           deprecated2[16]; // 16 bytes
    anytype                 max;             // 8 bytes
    unsigned char           deprecated3[16]; // 16 bytes
    double                  scale;           // 8 bytes
    unsigned char           deprecated4[16]; // 16 bytes
    double                  offset;          // 8 bytes
    unsigned char           deprecated5[16]; // 16 bytes
    char                    description[32]; // 32 bytes
};                                           // total of 192 bytes

Proposed new definition (new/modified fields marked with >):

struct EXTRA_BYTES {
>   unsigned short          extrabyte_id;    // 2 bytes
    unsigned char           data_type;       // 1 byte
    unsigned char           options;         // 1 byte
    char                    name[32];        // 32 bytes
    anytype                 no_data;         // 8 bytes
>   double                  min;             // 8 bytes
>   double                  max;             // 8 bytes
    double                  scale;           // 8 bytes
    double                  offset;          // 8 bytes
    char                    description[32]; // 32 bytes
};                                           // total of 108 bytes
abellgithub commented 2 years ago

As someone who writes the code to read and write such things, I see no benefit to this. The size in bytes of the extra bytes fields are trivial.

hobu commented 2 years ago

Additionally, software would have branch to support this additional layout. That equals more complex software that has a chance to break. What is the benefit of adding this other than to save a few bytes?

esilvia commented 2 years ago

The main benefits I see are being able to change the min/max data type from anytype to double so that it's consistent with the LAS header's convention for XYZ min/max, and to emphasize the importance of the Standard extrabyte_id.

I don't feel strongly about this, but I've had mixed feelings about whether we're allowed to repurpose those first two bytes in the existing ExtraByte VLR descriptor. This would bypass that question.

esilvia commented 2 years ago

Some have also commented in the past whether the options field is even relevant, since a 1.0 scale and 0.0 offset are synonymous with having none at all, so this could be an opportunity to refactor that.

Deguerre commented 2 years ago

I would be against removing scale and offset in the options field, because some field types are not semantically "numbers" and it doesn't make sense to do arithmetic on them. Also remember that 8-byte integers cannot be losslessly converted to doubles and back again.

esilvia commented 2 years ago

Consensus seems to be that this provides no benefit. I'm closing this as "won't do" and can be reopened at a later date if a reason comes up.