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

Add "E57Simple" API #41

Merged
merged 38 commits into from
Jun 10, 2020
Merged

Add "E57Simple" API #41

merged 38 commits into from
Jun 10, 2020

Conversation

ptc-jhoerner
Copy link
Contributor

Hi,

this is a port of E57Simple API of the reference library.

Main goal of this is to simplify reading and writing of point clouds with the most common attributes: float x, y, z; float nx, ny, nz; uint8 r, g, b (and any subset of these).

I have removed all the platform specific dependencies from the code, removed the unused code and formatted the code to fit the rest of the library. All duplicate documentation comments have been removed and the doxygen documentation is collected in E57Simple.h.

The main changes compared to the original implementation are:

  • removed GPS time conversions and the accompanying library (this must be handled on the user side)
  • automatically generate GUIDs when writing the data (unless we have user-provided GUIDs)
  • make sure all the classes of the simple API are always initialised (gives the user a bit more flexibility when working with metadata classes)
  • change data types to fit the most common use-cases -> point coordinates changed to floats (were double), colours are uint8 (were uint16) etc.
  • add support for E57_EXT_surface_normals
  • use int64_t for all sizes as in the current API
  • add equality operators for metadata structs (allows to consistently check whether a field is present or not by comparing to a default initialised struct)
  • introduced Data3DPointsData to simplify calls to SetUpData3DPointsData

You might want to merge this with squashing all the changes. I have kept here the commits to indicate the changes made to the original code.

Cheers,
Jiri

image
Jiri Hörner
Software Development Engineer, Vuforia

jhoerner@ptc.com

vuforia.com


Parametric Technology Gesellschaft m.b.H., Operngasse 17-21, 1040 Wien, Austria. Firmensitz: Wien, FN: 111171 m, Firmenbuchgericht: Handelsgericht Wien.

The information contained in this email transmission is confidential and may be privileged. If you are not the intended recipient, any use, dissemination, distribution, publication, or copying of the information contained in this email is strictly prohibited. If you have received this email in error, please immediately notify me by calling the above number and delete the email from your system. Thank you for your co-operation.

Copyright © 2020 PTC Inc. and/or all its affiliates or subsidiaries. All rights reserved

include/E57Simple.h Outdated Show resolved Hide resolved
@asmaloney
Copy link
Owner

As you can tell from the work I put into the original fork, I'm not a fan of "put everything in one file".

Would it be feasible to at least split this into two - one for read (E57SimpleRead) and one for write (E57SimpleWrite)?

Copy link
Owner

@asmaloney asmaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also a couple of easy fixes in the CodeFactor CI (blank lines). And maybe you can take a look at what it has identified as complex methods to see if they can be made less complex?

include/E57Simple.h Outdated Show resolved Hide resolved
include/E57Simple.h Outdated Show resolved Hide resolved
src/E57Simple.cpp Outdated Show resolved Hide resolved
src/E57SimpleImpl.h Outdated Show resolved Hide resolved
@ptc-jhoerner
Copy link
Contributor Author

Thanks a lot for the feedback and good suggestions. I will look into it next week and get back to you.

@asmaloney
Copy link
Owner

Thanks for the submission!

generate uuids
todo: time handling
* do not require optional coordinateMetadata
* generate new guid automatically when writing scans
Add support for normals as per E57_EXT_surface_normals. Currently only writing of normals as float32 is supported.
Follows the sizes used in the Foundation API.
default member initializers prevent aggregate initialization until C++14
* remove duplicate documentation - consolidate docuemntation in E57Simple.h
virtuality there is unused as no classes inherits from these
* remove extra semicolons
These callbacks don't fit the aim from the E57Simple API. For writing custom extensions use the Foundation API.
@ptc-jhoerner
Copy link
Contributor Author

I have rebased the branch to sync with master.

Aside the changes to address the comments above, I have also added the necessary E57_DLL declarations as the shared library changes has been merged now.

The code is now split into 2 files. I have tried to improve the code complexity side as well, but clearly most of the code is still based on the original implementation. I have at least removed some of those extremely long if cases in Reader by employing early exits there. That way it is also more consistent with the Writer.

Many of the other cleanups discussed above went really well. Thanks for those suggestions!

Copy link
Owner

@asmaloney asmaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - looking good!

A few changes requested and a couple of questions/suggestions.

include/E57Simple.h Outdated Show resolved Hide resolved
include/E57Simple.h Outdated Show resolved Hide resolved
include/E57Simple.h Outdated Show resolved Hide resolved
src/ReaderImpl.cpp Show resolved Hide resolved
src/ReaderImpl.h Outdated Show resolved Hide resolved
src/WriterImpl.cpp Outdated Show resolved Hide resolved
src/WriterImpl.cpp Outdated Show resolved Hide resolved
src/WriterImpl.cpp Outdated Show resolved Hide resolved
src/WriterImpl.h Outdated Show resolved Hide resolved
src/WriterImpl.h Show resolved Hide resolved
src/Common.cpp Outdated Show resolved Hide resolved
@ptc-jhoerner
Copy link
Contributor Author

Hi, sorry for a slow response from my side. Thanks for the feedback! I'm looking into the changes and I will get back to you tomorrow.

@asmaloney
Copy link
Owner

No problem at all Jiri!

I appreciate you taking the time to work on this and that you are willing to incorporate feedback.

automatic fix with clang-tidy
@ptc-jhoerner
Copy link
Contributor Author

I'm done with the last round of changes. Thanks for all he suggestions. Most of them I have just applied directly.

I took a broader look at the constness of the member methods of both Reader and Writer and I have changed all of them to follow the semantic constness w.r.t. E57 file. Constness is also now the same for the Impl classes.

I have also applied readability-braces-around-statements clang-tidy check and I have split the API as suggested, so you might get a bigger diff again.

Cheers,
Jiri

@ptc-jhoerner ptc-jhoerner marked this pull request as draft June 10, 2020 09:55
@ptc-jhoerner ptc-jhoerner marked this pull request as ready for review June 10, 2020 11:09
Copy link
Owner

@asmaloney asmaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your work on this Jiri!

@asmaloney asmaloney merged commit ad7af78 into asmaloney:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants