asmaloney / asmaloney/libE57Format

Simple Reader/Writer PointRecord Representation Issues

Open
#157 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
191
Forks
82
Avg merge
7h 21m
Merged PRs (30d)
1

Description

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](https://github.com/asmaloney/libE57Format/pull/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](https://github.com/asmaloney/libE57Format/pull/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](https://github.com/asmaloney/libE57Format/pull/167)).
* ~~All colours are stored as uint8_t, so files using uint16_t [will fail to load](https://github.com/asmaloney/libE57Format/issues/159).~~ (Fixed by [#167](https://github.com/asmaloney/libE57Format/pull/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.

Contributor guide

Open the contributing guide

Research direction

Start by reading the Simple API read/write code and its handling of PointStandardizedFieldsAvailable, then compare the relevant PointRecord node types with the standard tables cited in the issue. The issue lists several distinct gaps but does not identify specific files or tests; done would require narrowing and addressing the agreed-upon type-handling gaps.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
data
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.