-
Notifications
You must be signed in to change notification settings - Fork 319
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
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@wayfarer3130 |
I will review it this week |
@sedghi |
can you check if these changes are merged your PR is needed or not anymore? #1717 |
This PR still needs to be merged because the issue I mentioned in the context section occurs even when |
@sedghi |
I will come back to this soon, sorry for the delay |
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
Changes & Results
This issue seems to be caused by an incorrect
tVoiRange
value, which is calculated usingwindowWidth
. To fix this, I replaced thewindowWidth
value with a different calculation:Math.abs(voiRange.upper - voiRange.lower)
, instead of relying on thewindowWidth
provided by thetoWindowLevel
function.Initially, I suspected that the issue might originate in the
toWindowLevel
function itself, because it calculateswindowWidth
using the formulaMath.abs(high - low) + 1
. My assumption was that it should useMath.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 byMath.abs(high - low)
for this specific issue.Testing
I visually confirmed that this issue has been resolved using
examples/colorbar/index.ts
.Checklist
PR
semantic-release format and guidelines.
Code
My code has been well-documented (function documentation, inline comments,
etc.)
I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment