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

fix(Colors): fix UInt8Clamped array color definition #3034

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

rodrigobasilio2022
Copy link
Contributor

@rodrigobasilio2022 rodrigobasilio2022 commented Mar 19, 2024

Context

This PR fixes a bug in vtk.js when one creates a color definition with UInt8Clamped array structure

Results

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js:
    • OS:
    • Browser:

@rodrigobasilio2022
Copy link
Contributor Author

rodrigobasilio2022 commented Mar 19, 2024

@finetjul This PR i solves the problem with color defined with UInt8Clamped arrays

@@ -15,6 +15,7 @@ export const VtkDataTypes = {
CHAR: 'Int8Array',
SIGNED_CHAR: 'Int8Array',
UNSIGNED_CHAR: 'Uint8Array',
UNSIGNED_CHAR_CLAMPED: 'Uint8ClampedArray',
Copy link
Member

Choose a reason for hiding this comment

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

@jourdain @floryst could this have an impact with VTK comptability ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is for UCHAR we have two possible values...

Copy link
Collaborator

Choose a reason for hiding this comment

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

My first guess is that this won't affect VTK compatibility, since these constants need to be mapped to the VTK equivalents anyways.

Copy link
Member

Choose a reason for hiding this comment

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

@rodrigobasilio2022 maybe add an inline comment that says it "should be used for VTK.js internal purpose only (e.g.GPU)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@finetjul Done squash

Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

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

LGTM (the 2 commits could be squashed though...)

@rodrigobasilio2022
Copy link
Contributor Author

LGTM (the 2 commits could be squashed though...)

Done quash

@finetjul finetjul added this pull request to the merge queue Mar 20, 2024
Merged via the queue into Kitware:master with commit bfa31f7 Mar 20, 2024
3 checks passed
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.

3 participants