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(ColorbarCanvas): Fix colorbar rendering issue on small window widths #1649

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stada526
Copy link
Contributor

@stada526 stada526 commented Dec 2, 2024

Context

When the window width is small, the colorbar doesn't render properly, with the top part appearing gray instead of white.

2024-12-02.13.06.54.mov
スクリーンショット 2024-12-02 13 07 26 スクリーンショット 2024-12-02 13 07 34

Changes & Results

This issue seems to be caused by an incorrect tVoiRange value, which is calculated using windowWidth. To fix this, I replaced the windowWidth value with a different calculation: Math.abs(voiRange.upper - voiRange.lower), instead of relying on the windowWidth provided by the toWindowLevel function.

Initially, I suspected that the issue might originate in the toWindowLevel function itself, because it calculates windowWidth using the formula Math.abs(high - low) + 1. My assumption was that it should use Math.abs(high - low) instead. However, I found this PR #736 that says this formula aligns with the DICOM standard. While I don't fully understand why the DICOM standard is like this - since it goes against my intuition - it is clear that this calculation doesn't work well for colorbar rendering.

As a result, I only replaced the windowWidth variable with the value calculated by Math.abs(high - low) for this specific issue.

Testing

I visually confirmed that this issue has been resolved using examples/colorbar/index.ts.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • "OS: maxOS 15.1"
  • "Node version: v21.7.1"
  • "Browser: Chrome 130.0.6723.117"

Copy link

stackblitz bot commented Dec 2, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 9dbf48c
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/674e044cd58be30008b2f48b
😎 Deploy Preview https://deploy-preview-1649--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sedghi sedghi requested a review from wayfarer3130 December 3, 2024 14:12
@stada526
Copy link
Contributor Author

@wayfarer3130
Hi, do you have any plans to review this PR?
I’d really appreciate it if you could take some time to look at it soon.

@sedghi sedghi requested review from sedghi and removed request for wayfarer3130 December 18, 2024 16:55
@sedghi
Copy link
Member

sedghi commented Dec 18, 2024

I will review it this week

@stada526
Copy link
Contributor Author

@sedghi
I really appreciate it 🙏

@sedghi
Copy link
Member

sedghi commented Dec 18, 2024

can you check if these changes are merged your PR is needed or not anymore? #1717

@stada526
Copy link
Contributor Author

This PR still needs to be merged because the issue I mentioned in the context section occurs even when windowWidth or windowCenter are more than 1.

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