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

Add config for getGPUTier method in cornerstone.init #633

Merged
merged 9 commits into from
Jul 4, 2023

Conversation

iamiota
Copy link
Contributor

@iamiota iamiota commented May 26, 2023

Context

When cornerstone3D is being initialized, the getGPUTier method from detect-gpu will generate a network request, which will not be implemented within a LAN and at the same time will block the initialization of cornerstone3D.

Move the parameter of getGPUTier to the parameter of cornerstone3D.init, users can configure the path of device information by themselves. For example, they can download the device information files to a specific path on the hard disk in advance, so that even in a LAN, files can still be requested.

Perhaps the name getGPUTierConfig is not accurate enough, do you have any suggestions?

Changes & Results

import * as cornerstone from '@cornerstonejs/core'
cornerstone.init({
  getGPUTierConfig: {
    benchmarksURL: '/resources/devices',
  },
})

Before

WX20230526-145648@2x

After

WX20230526-145712@2x

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: macOS 12.6.2
  • [] "Node version: 16.13.0
  • [] "Browser: Chrome 113.0.5672.126

* 'main' of github.com:iamiota/cornerstone3D-beta: (59 commits)
  chore(release): publish [skip ci]
  fix(event): Interactions between double click and multi mouse button (cornerstonejs#616)
  chore(release): publish [skip ci]
  fix(windowLevel): should not double scale for window level (cornerstonejs#619)
  chore(release): publish [skip ci]
  fix(expose): api default mouse primary (cornerstonejs#622)
  chore(release): publish [skip ci]
  docs(typo): in imageLoader.md (cornerstonejs#629)
  chore(release): publish [skip ci]
  fix(husky): husky config (cornerstonejs#627)
  chore(release): publish [skip ci]
  feat(api): cornerstone3D (cornerstonejs#626)
  chore(release): publish [skip ci]
  feat(api)!: cornerstone3D version 1.0 (cornerstonejs#625)
  chore(release): publish [skip ci]
  feat(version1)!: publish version 1.0 (cornerstonejs#624)
  chore(release): publish [skip ci]
  fix(versioning): sync all versions (cornerstonejs#623)
  chore(release): publish [skip ci]
  fix(cpu): could not render if switched to cpu in the middle (cornerstonejs#615)
  ...
…rmation files in the Internet from within the local network(LAN).
@netlify
Copy link

netlify bot commented May 26, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64a418bb29fb2709ead08f46
😎 Deploy Preview https://deploy-preview-633--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
Copy link
Member

sedghi commented Jun 14, 2023

Hey, sorry I was super busy with all the releases.
I really like the idea. But I don't see the implementation would work. I think this is what you are trying to do

the init function would be used like this

cornerstone3D.init({getGPUConfig: ()=>{}})

then inside our init you should check if the function is provided and use that instead of the network request and bypass the detect-gpu.

Let me know what you think.
Also we should update the docs to reflect this change after

@iamiota
Copy link
Contributor Author

iamiota commented Jun 21, 2023

I'm sorry, I didn't express myself clearly.
I don't want to generate a network request to replace getGPUTier.

First, I looked at the detect-gpu documentation and found out that the getGPUTier function allows an object parameter to be passed in, including a property called benchmarksURL, which overrides the default device information request address (the default address is the request address shown in the first screenshot).

So what I want to do is pass the benchmarksURL property to getGPUTier through the cornerstone.init function. The code I submitted is effective, and the screenshot above is the result of my local verification.

I don't know if it's elegant to pass like this in the cornerstone.init, please forgive my poor English.
Thanks!

@sedghi
Copy link
Member

sedghi commented Jun 22, 2023

@iamiota I see ok makes sense. Looking back at this detectGPU does not makes sense, it should be renamed to gpuTier since it holds the tier, and you can add the detectGPUConfig and add your benchmark url

@iamiota
Copy link
Contributor Author

iamiota commented Jun 29, 2023

After careful consideration, I have resubmitted some code. Overall, the description of this PR is as follows:

  1. Renamed detectGPU to gpuTier and specified its type as TierResult(which is the return value of getGPUTier), it is optional. Users could obtain device information in their own way and pass it as a parameter to cornerstone3D.init. When this value exists, cornerstone3D.init method will use this parameter instead of making a network request to retrieve device information.

  2. Added the detectGPUConfig parameter, which is of type GetGPUTier (the parameter for getGPUTier). This allows users to configure it based on their network conditions.

Thanks! @sedghi @sbsurf

@iamiota iamiota requested a review from sbsurf June 30, 2023 08:14
type Cornerstone3DConfig = {
detectGPU: any;
gpuTier?: TierResult;
Copy link
Member

Choose a reason for hiding this comment

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

can you please add some documentation here for these two properties?

for gpuTier, that we use it if provided if not a network call is performed

more important one is the detectGPUConfig with some reference to the detect-gpu repo of what confis are available

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, it has already been added.

@sedghi
Copy link
Member

sedghi commented Jul 4, 2023

You should run yarn build:update-api and include(commit) the results. I have done that for you this time, just FYI for next time

@sedghi sedghi merged commit b5ea0e2 into cornerstonejs:main Jul 4, 2023
5 checks passed
@lecion lecion mentioned this pull request Sep 4, 2023
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