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

Prototypes failing range check when writing #274

Open
RiskyRob opened this issue Jan 10, 2024 · 3 comments
Open

Prototypes failing range check when writing #274

RiskyRob opened this issue Jan 10, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@RiskyRob
Copy link

A similar issue to #246 but different.

In file src/WriterImpl.cpp in method NewData3D we are creating prototypes, e.g.

proto.set( "colorGreen", IntegerNode( imf_, 0, (int64_t)data3DHeader.colorLimits.colorGreenMinimum, (int64_t)data3DHeader.colorLimits.colorGreenMaximum ) );

The problem is that the minimum bounds has to be 0 or this code will fail because the default value being passed in for this prototype is 0. This makes the minimum bounds property effectively an unusable property for many data fields as it always has to be 0. When the minimum bounds is greater than 0 the software will crash with an E57_ERROR_VALUE_OUT_OF_BOUNDS exception.

As stated in #246 for prototypes (E57 Standard 8.3.9.3 (1)):

(1) The prototype child element specifies the structure of the data that will be stored in the CompressedVector, as well as the possible range of values that the data may take. The prototype shall be any E57 element type (with potential sub-children) except Blob and CompressedVector. The values of the prototype elements and sub-elements are ignored, and need not be specified.

And in the header of the IntegerNode class:

Warning: it is an error to give an @a value outside the @a minimum / @a
maximum bounds, even if the IntegerNode is destined to be used in a
CompressedVectorNode prototype (where the @a value will be ignored). If the
IntegerNode is to be used in a prototype, it is recommended to specify a @a
value = 0 if 0 is within bounds, or a @a value = @a minimum if 0 is not within
bounds
@asmaloney asmaloney added the bug Something isn't working label Jan 10, 2024
@asmaloney asmaloney changed the title Prototypes failing range check Prototypes failing range check when writing Jan 10, 2024
@asmaloney
Copy link
Owner

I've spent some time looking into this and it seems like it's a bit of an overall design problem.

Whoever initially wrote this code added checks against the ranges in the node constructors (which throw exceptions). Add to that a bunch of default arguments to constructors and that the XML generation is entwined with the nodes and it's a bit of a mess.

Not yet sure how to solve it nicely.

@RiskyRob
Copy link
Author

Agreed the original creators could have considered using specific templating functionality or something! For my purposes I've set the default argument as the minimum value for the prototypes where 0 could cause problems but this is certainly not the ideal solution, it is just a quick work around unfortunately.

The work that you have done has been incredible btw, nicely done!

@asmaloney
Copy link
Owner

I've set the default argument as the minimum value for the prototypes

This was my initial "fix" as well, but it then means that (in the colour case) the colorLimits must be specified and that's actually an optional field. Its meaning in the standard is such that it can't be computed from the data:

The limits for the value of red, green, and blue color that the sensor is capable of producing.

(It also looks like leaving the defaults for IntegerNode min and max leads to UB.)

I'm still trying to work out if I can fix this without breaking the API...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants