asmaloney / asmaloney/libE57Format
Simple Reader/Writer PointRecord Representation Issues
- 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
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