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(iOS): Implement texture handling for iOS issues in late 16 and 17 #1212

Merged
merged 17 commits into from
Apr 18, 2024

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Apr 17, 2024

Context

There were couple of bugs reported issues for cornerstone on iOS specifically late 16 and 17.

As discussed in

Kitware/vtk-js#3027
#1133

The issue is iOS does not have support for OES_texture_float_linear (https://developer.mozilla.org/en-US/docs/Web/API/OES_texture_float_linear) but this feature was wrongly reported as true in the older iOS (https://bugs.webkit.org/show_bug.cgi?id=264404#c2) and from 16.5 ish iOS is throwing errors.

We have two other tools for not rendering floats

  1. The 16 bit texture extension via https://developer.mozilla.org/en-US/docs/Web/API/EXT_texture_norm16 which has decent support https://web3dsurvey.com/webgl2/extensions/EXT_texture_norm16, and if exists we should use it
  2. preferSizeOverAccuracy: which originally was added for memory management, but under the hood is uses half float textures, and mostly works across the devices, though as the name suggest it has some inaccuracies in high bits

So This PR will detect if the host device is mobile and set the 16 bit texture (if available) if not it will try to do preferSizeOverAccuracy


Limitations

Although this PR addresses the rendering of the Stack and Volume Viewports, for scenarios like PT SUV scale, we require Float32Array as we can't render via 16-bit norms. Therefore, we detect this and simply render the PT in a non-scaled fashion. It would be ideal to find a proper solution to address this final issue.

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:
  • [] "Node version:
  • [] "Browser:

Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 9ebbcf0
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/66203e0b1cd5c100083ea99b
😎 Deploy Preview https://deploy-preview-1212--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.

…eateImage, stackPrefetch, stackContextPrefetch, and loadImageToCanvas
…eateImage, stackPrefetch, stackContextPrefetch, loadImageToCanvas, generateVolumePropsFromImageIds, hasFloatScalingParameters, and hasFloatRescale
@sedghi sedghi changed the title feat(iOS): Implement texture handling for iOS issues in late 16 and 17 fix(iOS): Implement texture handling for iOS issues in late 16 and 17 Apr 17, 2024
@sedghi sedghi requested a review from IbrahimCSAE April 17, 2024 21:20
Copy link
Collaborator

@IbrahimCSAE IbrahimCSAE left a comment

Choose a reason for hiding this comment

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

Great fix, much needed

@IbrahimCSAE IbrahimCSAE merged commit 2191ada into main Apr 18, 2024
9 checks passed
@Congritulation
Copy link

Congritulation commented Apr 20, 2024

Excuse me. I run my project in IPadOS 17.4.1 with cornerstone3D 1.70.10 and encounter with the same situation. The volumeViewport show nothing but a gray rectangle. so what i want to ask is [Bug] Volume viewports not working on ios #1184 fixed and released before v1.70.10?
@sedghi

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