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

E57SimpleWriter API setup & writing cleanup? #163

Closed
asmaloney opened this issue Nov 4, 2022 · 1 comment · Fixed by #171
Closed

E57SimpleWriter API setup & writing cleanup? #163

asmaloney opened this issue Nov 4, 2022 · 1 comment · Fixed by #171
Labels

Comments

@asmaloney
Copy link
Owner

Right now to write data we need to do the following:

e57::Data3D header;
// set up the header

e57::Data3DPointsData pointsData( header );
// set the point data

const int64_t scanIndex = writer->NewData3D( header );

e57::CompressedVectorWriter dataWriter = writer->SetUpData3DPointsData( scanIndex, cNumPoints, pointsData );

dataWriter.write( cNumPoints );
dataWriter.close();

This does not seem as simple as it should be for a simple API. It's easy to miss a step and it's not always clear what's wrong. For example if you forget to dataWriter.close(), it doesn't fail but it also doesn't write the data.

Why not collapse it into:

void WriteData3D( Data3D &data3DHeader, size_t pointCount, const Data3DPointsData &buffers );

(The same can be done for Image2D - collapse NewImage2D into WriteImage2DData.)

Then:

e57::Data3D header;
// set up the header

e57::Data3DPointsData pointsData( header );
// set the point data

writer->WriteData3D( header, cNumPoints, pointsData );

Does anyone have any idea why it's split up the way it is?

asmaloney added a commit that referenced this issue Nov 4, 2022
Collapse some steps into one call to hide complexity, avoid potential errors, and simply the use of the API.

Fixes #163
asmaloney added a commit that referenced this issue Nov 6, 2022
Collapse some steps into one call to hide complexity, avoid potential errors, and simplify the use of the API.

Fixes #163
asmaloney added a commit that referenced this issue Nov 8, 2022
Collapse some steps into one call to hide complexity, avoid potential errors, and simplify the use of the API.

Fixes #163
asmaloney added a commit that referenced this issue Nov 8, 2022
Collapse some steps into one call to hide complexity, avoid potential errors, and simplify the use of the API.

Fixes #163
@nigels-com
Copy link
Contributor

I suspect there was a reason the API was the way it was.
We're writing to E57 in batches, rather than all at once.
Our point clouds can be very large, would much rather not have two complete copies in memory at once.

e57::Data3D header;
e57::Data3DPointsData pointsData( header );

const int64_t scanIndex = writer->NewData3D( header );

e57::CompressedVectorWriter dataWriter = writer->SetUpData3DPointsData( scanIndex, totalPoints, pointsData );
// (copy first batch of data)
dataWriter.write( batchSize );
// (copy next batch of data)
dataWriter.write( batchSize );
// (copy next batch of data)
dataWriter.write( batchSize );
// (copy next batch of data)
dataWriter.write( batchSize );
// (copy next batch of data)
dataWriter.write( batchSize );
...
dataWriter.close();

So my conundrum now is whether to comment-out the deprecation so we can at least come forward to version 3.
I appreciate there is an architectural question of how "simple" a SimpleWriter should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants