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(CrosshairsTool): support HPDI screens in CrosshairsTool #1824

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

daker
Copy link
Contributor

@daker daker commented Feb 13, 2025

fix #1822

Context

Support HDPI screens in CrosshairsTool

Changes & Results

Before After
before aftre

Testing

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: Windows 11
  • Node version:
  • Browser: Chrome 133.0.6943.59

@daker daker changed the title fix: support HPDI screens in CrosshairsTool fix(CrosshairsTool): support HPDI screens in CrosshairsTool Feb 13, 2025
Copy link

netlify bot commented Feb 13, 2025

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit c19d075
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67c5e1d82291cd0008f62f81
😎 Deploy Preview https://deploy-preview-1824--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.

@daker daker force-pushed the fix-crosshairstool-hdpi-screens branch from 5e1c053 to 729d21f Compare February 13, 2025 13:45
@daker
Copy link
Contributor Author

daker commented Feb 19, 2025

@sedghi can you take a look at this fix ?

@sedghi
Copy link
Member

sedghi commented Feb 19, 2025

Thanks for the PR! I see the visuals have changed quite a bit, and I'm not sure if we want to push that onto all users. Is there a way to implement your PR without such a significant visual change? Or, could we perhaps reduce the size of those handles? They're really large on my screen right now.

@daker
Copy link
Contributor Author

daker commented Feb 19, 2025

Can you share a screenshot ? because i didn't changed the styles, only the size of the handles multiplied by window.devicePixelRatio.

Copy link
Member

sedghi commented Feb 19, 2025

The screenshot is the same as the one above. What I mean is that there seems to be a big visual difference. Maybe the handleRadius (currently 2 and 3 in different places) should be configurable so that if users don't like large icons, they can reduce it.

@daker
Copy link
Contributor Author

daker commented Feb 23, 2025

what do you think about supportHDPI in configration ?

@sedghi
Copy link
Member

sedghi commented Feb 24, 2025

I was kind of thinking along these lines:

let handleRadius = this.configuration.handleRadius * window.devicePixelRatio;

same for the other one

@daker daker force-pushed the fix-crosshairstool-hdpi-screens branch from 729d21f to da8d602 Compare February 27, 2025 11:12
@daker
Copy link
Contributor Author

daker commented Feb 27, 2025

@sedghi ready for review

@daker
Copy link
Contributor Author

daker commented Mar 3, 2025

@sedghi is it ok for you ?

@sedghi
Copy link
Member

sedghi commented Mar 3, 2025

sorry for the delay, can we add another configuration about the HDPI screens too? like enableHDPIHandles

@daker daker force-pushed the fix-crosshairstool-hdpi-screens branch from da8d602 to c19d075 Compare March 3, 2025 17:07
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Thanks!

@sedghi sedghi merged commit cd299e4 into cornerstonejs:main Mar 3, 2025
17 of 19 checks passed
@daker daker deleted the fix-crosshairstool-hdpi-screens branch March 3, 2025 21:08
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.

[Feature Request] How to enlarge CrosshairsTool's handle?
2 participants