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

Add compact ExtraByte VLR v2 #119

Closed
esilvia opened this issue Mar 3, 2022 · 6 comments
Closed

Add compact ExtraByte VLR v2 #119

esilvia opened this issue Mar 3, 2022 · 6 comments

Comments

@esilvia
Copy link
Member

esilvia commented Mar 3, 2022

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
@esilvia esilvia added this to the v1.5 milestone Mar 3, 2022
@abellgithub
Copy link

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
Copy link
Contributor

hobu commented Mar 4, 2022

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
Copy link
Member Author

esilvia commented Mar 4, 2022

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
Copy link
Member Author

esilvia commented Mar 4, 2022

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
Copy link

Deguerre commented Apr 7, 2022

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
Copy link
Member Author

esilvia commented May 16, 2022

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.

@esilvia esilvia closed this as completed May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants