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

Simple API implementation of E57_EXT_surface_normals doesn't handle scaled ints #150

Open
asmaloney opened this issue Oct 28, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@asmaloney
Copy link
Owner

asmaloney commented Oct 28, 2022

In Data3DPointsData_t, the fields for E57_EXT_surface_normals are declared as floats:

float *normalX = nullptr; //!< The X component of a surface normal vector (E57_EXT_surface_normals extension).
float *normalY = nullptr; //!< The Y component of a surface normal vector (E57_EXT_surface_normals extension).
float *normalZ = nullptr; //!< The Z component of a surface normal vector (E57_EXT_surface_normals extension).

And they are explicitly written as floats:

// currently we support writing normals only as float32
if ( data3DHeader.pointFields.normalXField )
{
   proto.set( "nor:normalX", FloatNode( imf_, 0., E57_SINGLE, -1., 1. ) );
}

I'm not sure why this restriction is here, but the spec for the extension allows for both floats and doubles (and uses doubles in their examples).

Edit: Should look at supporting scaled ints (see below).

@asmaloney asmaloney added the bug Something isn't working label Oct 28, 2022
@ptc-jhoerner
Copy link
Contributor

I'm not sure whether there is any real need to have normals in doubles. Normals should be of unit length and float has more than enough precision within [-1, 1] range to capture normals for the point precision achievable with the best scanners. The example uses ScaledIntegerNode, which might be worth supporting, quantisation of normals is quite common technique to save space. With that said, I'm not sure how many users who are happy to quantise would use E57, they might want to use draco compression anyway.

@asmaloney
Copy link
Owner Author

The example uses ScaledIntegerNode...

Ah quite right! I missed that - I just saw all the doubles everywhere 😄

Adding a way for it to use scaled ints seems like it should be straightforward and would make sense to me.

@asmaloney asmaloney changed the title Simple API implementation of E57_EXT_surface_normals doesn't handle doubles Simple API implementation of E57_EXT_surface_normals doesn't handle scaled ints Oct 31, 2022
@asmaloney asmaloney added enhancement New feature or request and removed bug Something isn't working labels Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants