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

Added remaining color types for PNTS #391

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

pizza3
Copy link
Contributor

@pizza3 pizza3 commented Jul 8, 2023

Added support for rgba, rgb565 and constant_rgba for PNTSLoader.

The example data used for rgb rgb565 constant_rgba

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Thanks! A couple comments - I'll give this a test after those are handled.

const color = new Color( `rgb(${CONSTANT_RGBA[ 0 ]}, ${CONSTANT_RGBA[ 1 ]}, ${CONSTANT_RGBA[ 2 ]})` );
material.color = color;
material.opacity = CONSTANT_RGBA[ 3 ] / 255;
material.transparent = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be set to true if the opacity is < 1.0

Comment on lines 3 to 8
let r = ( rgb565 & 0xF800 ) >> 11;
let g = ( rgb565 & 0x07E0 ) >> 5;
let b = rgb565 & 0x001F;
r = ( r << 3 ) | ( r >> 2 ); // Scale up red component
g = ( g << 2 ) | ( g >> 4 ); // Scale up green component
b = ( b << 3 ) | ( b >> 2 ); // Scale up blue component
Copy link
Contributor

Choose a reason for hiding this comment

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

These all seem like magic numbers. Does this 565 unpacking implementation come from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I updated the 565 unpacking to a much cleaner one I found over here link, the previous one was taken from a arduino forum.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks! Can we add a comment with a link to that page - then I'll test it and get this merged!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok 👍

@gkjohnson
Copy link
Contributor

Made a few small changes - but this looks great! Thanks.

@gkjohnson gkjohnson merged commit 318ec91 into NASA-AMMOS:master Sep 27, 2023
2 checks passed
@gkjohnson gkjohnson added this to the v0.3.21 milestone Sep 27, 2023
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.

2 participants