Simple Reader/Writer PointRecord Representation Issues

[asmaloney]

After digging around the Simple API read/write code, here are some findings about type selection of various nodes.

The standard has the following info about node types for various PointRecord fields (abbreviated table 14):

Element	Float (float)	Float (double)	ScaledInteger	Integer	Notes
cartesianX	✅	✅	✅	❌
cartesianY	✅	✅	✅	❌
cartesianZ	✅	✅	✅	❌
sphericalRange	✅	✅	✅	❌
sphericalAzimuth	✅	✅	✅	n/a
sphericalElevation	✅	✅	✅	n/a
timeStamp	✅	✅	✅	✅
intensity	✅	✅	✅	✅	1
colorRed	❌	❌	❌	✅	2
colorGreen	❌	❌	❌	✅	2
colorBlue	❌	❌	❌	✅	2

1. Added Float (double) in 3.0. (#178)
2. Reads the all colour node types and converts to uint16_t, but can only write using Integer.

✅ = supported by Simple API ❌ = not supported by Simple API n/a = not supported by standard

There are 4 fields in Simple API's PointStandardizedFieldsAvailable for setting information about how to write this data. These are:

• pointRangeScaledInteger applies to cartesianX, cartesianY, cartesianZ, & sphericalRange, and allows FloatNode/ScaledIntegerNode
• angleScaledInteger applies to sphericalAzimuth & sphericalElevation, and allows FloatNode/ScaledIntegerNode
• timeScaledInteger applies to timeStamp and allows FloatNode/ScaledIntegerNode/IntegerNode
• intensityScaledInteger applies to intensity and allows FloatNode/ScaledIntegerNode/IntegerNode

Setting pointRangeScaledInteger or angleScaledInteger to:

• 0.0 uses ScaledIntegerNode (E57_NOT_SCALED_USE_FLOAT)
• < 0.0 uses FloatNode w/doubles
• > 0.0 uses FloatNode w/floats

timeScaledInteger handles node type selection differently since it allows IntegerNode. So:

• > 0.0, use ScaledIntegerNode
• == 0.0 AND (timeMaximum == E57_FLOAT_MAX), use FloatNode w/floats
• == 0.0 AND (timeMaximum == E57_DOUBLE_MAX), use FloatNode w/doubles
• == 0.0 AND timeMaximum neither of those fails to write anything
• < 0.0, use IntegerNode

intensity handles node type selection differently still (no option to write doubles):

• > 0.0, use ScaledIntegerNode
• == 0.0, use FloatNode w/floats
• < 0.0, use IntegerNode

There are several issues with the current code:

• The standard allows cartesianX/cartesianY/cartesianZ to be IntegerNode which is not handled by either the reader or the writer.
• The standard allows cartesianX/cartesianY/cartesianZ to be different types (e.g. ScaledInteger/Float (double)/Float (float)), but the reader & writer do not allow this.
• ~~intensity doesn't allow writing FloatNode w/doubles which is allowed by the standard.~~ (Fixed by #178)
• The standard allows intensityMaximum and intensityMinimum (which are derived from intensity) to be different types (table 13), but the Simple API does not. Presumably the types are the same, but it's only recommended, not required by the standard.

  8.4.19.4 It is recommended that the element type of intensityMinimum and intensityMaximum be the same as the intensity element in the PointRecord.

• All colours are written as IntegerNodes, but read using their proper types and converted to ~~uint8_t~~ uint16_t (#167).
• ~~All colours are stored as uint8_t, so files using uint16_t will fail to load.~~ (Fixed by #167)
• Reading in intensityLimits doesn't give us enough information to set intensityScaledInteger properly since the scale can be different between intensityMaximum and intensityMinimum (it's written using the same scale in the Simple API, but that's not a requirement by the standard).

(Related to #126.)

Edit: Expanded table to show what Simple API supports.

[ollira]

Hi,

The old las2e57 tool writes E57 files that contain 16-bit color data. Currently E57SimpleReader throws an error, if a color element value is greater than 255:

ERROR: **** Got an e57 exception: a value could not be represented in the requested type (E57_ERROR_VALUE_NOT_REPRESENTABLE)
  Debug info: 
    context: pathName=colorRed value=65280
    sourceFunctionName: e57::SourceDestBufferImpl::setNextInt64

To fix these, I would like to suggest a new type definition for color data elements in E57SimpleData.h, something like:

    typedef uint16_t RgbElement;

    template <typename COORDTYPE = float> struct E57_DLL Data3DPointsData_t
    {
        ...
        RgbElement *colorRed = nullptr;
        RgbElement *colorGreen = nullptr;
        RgbElement *colorBlue = nullptr;

Substituting uint16_t for uint8_t would require application code changes.

Regards, Olli

[asmaloney]

@ollira I'm not opposed to this change, though I'll have to look at it a bit more carefully. I will create a new issue to track it and receive other input.

Substituting uint16_t for uint8_t would require application code changes.

Version 3 is the time to do it! 😄

Do you have a small (<10k) example you can send me to contribute to the testing data (under CC0 public domain license)?

[ollira]

Here is a small test file, down-sampled and converted ColouredCubeFloat.e57 :

ColouredCubeSample.zip

[asmaloney]

Excellent - thank you.