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

feat(PNTSLoader): support NORMAL and NORMAL_OCT16P semantics (#594) #900

Merged

Conversation

sguimmara
Copy link
Contributor

@sguimmara sguimmara commented Dec 30, 2024

This PR adds support for NORMAL and NORMAL_OCT16P (octahedron-encoded normals) point semantics to the PNTSLoader.

Resources:

fix #594

@sguimmara sguimmara force-pushed the pnts-support-normal-attributes branch from a75c084 to 8734a99 Compare December 30, 2024 10:02
@sguimmara sguimmara marked this pull request as ready for review December 30, 2024 10:02
@gkjohnson gkjohnson added this to the v0.4.1 milestone Dec 30, 2024

const f = new Vector2();

// https://stackoverflow.com/a/74745666/2704779
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the change! Can you explain where this function came from at this link? It looks like the SO page is primarily about going from a normal -> to oct encoded rather than reverse. Is this function directly ported from somewhere or is it your own implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I used the SO page to write the unit tests. The primary resource for the implementation itself is this blog post.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks - can you add that link to the inline comment, then, so it doesn't get lost? The implementation isn't otherwise so intuitive. Other than that it looks great, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: I added a jsdoc with the links

@sguimmara sguimmara force-pushed the pnts-support-normal-attributes branch from 8734a99 to 77b6fc7 Compare January 2, 2025 11:12
@sguimmara sguimmara force-pushed the pnts-support-normal-attributes branch from 77b6fc7 to 9836543 Compare January 2, 2025 13:44
@gkjohnson
Copy link
Contributor

Great, thanks!

@gkjohnson gkjohnson merged commit 82d89b1 into NASA-AMMOS:master Jan 2, 2025
2 checks passed
@sguimmara sguimmara deleted the pnts-support-normal-attributes branch January 14, 2025 09:24
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.

[PNTSLoader] Unsupported features with custom points material
2 participants