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

Colormap wasn't being applied #3464

Merged
merged 12 commits into from
Jul 4, 2023
Merged

Colormap wasn't being applied #3464

merged 12 commits into from
Jul 4, 2023

Conversation

wayfarer3130
Copy link
Contributor

@wayfarer3130 wayfarer3130 commented Jun 9, 2023

Context

Fixed the calls to set the PET colormaps to use the new format/correct set of values to be looked up.

Changes & Results

Colormaps can now be applied by selecting the menu item. Test changing the colormaps.
Removed the number from the colormap menu, using the default renderer so it shows correctly
Removed keyboard shortcuts 6-9 as they were causing exceptions due to invalid values

OpacityMapping is renamed to opacity since the API has changed in cornerstone3D

Testing

Run tmtv mode, change the colormap.

Checklist

PR

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

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

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

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

@netlify
Copy link

netlify bot commented Jun 9, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 5cfbdc5
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64a436f4bd1343000804963f
😎 Deploy Preview https://deploy-preview-3464--ohif-platform-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.

@netlify
Copy link

netlify bot commented Jun 9, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 5cfbdc5
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/64a436f4fbaac10008e2172c
😎 Deploy Preview https://deploy-preview-3464--ohif-dev.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.

@wayfarer3130 wayfarer3130 changed the title Fix/colormap tmtv fix(tmtv): Colormap wasn't being applied Jun 9, 2023
@wayfarer3130 wayfarer3130 requested a review from sedghi June 9, 2023 15:48
@wayfarer3130 wayfarer3130 changed the title fix(tmtv): Colormap wasn't being applied [WIP - needs CS3D] fix(tmtv): Colormap wasn't being applied Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #3464 (5cfbdc5) into master (3efa793) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3464   +/-   ##
=======================================
  Coverage   42.81%   42.81%           
=======================================
  Files          82       82           
  Lines        1448     1448           
  Branches      338      338           
=======================================
  Hits          620      620           
  Misses        665      665           
  Partials      163      163           
Impacted Files Coverage Δ
platform/core/src/defaults/hotkeyBindings.js 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce29bbe...5cfbdc5. Read the comment docs.

@wayfarer3130 wayfarer3130 changed the title [WIP - needs CS3D] fix(tmtv): Colormap wasn't being applied Colormap wasn't being applied Jun 29, 2023
@wayfarer3130 wayfarer3130 requested a review from jbocce June 29, 2023 15:08
@@ -36,13 +35,6 @@ function _createColormap(label, colormap) {
colormap,
},
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicated unnecessarily redundant.

@@ -345,7 +338,6 @@ const toolbarButtons = [
tooltip: 'PET Image Colormap',
},
isAction: true, // ?
renderer: WindowLevelMenuItem,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing the menu to have numbers beside it, causing it to look like it was appliable from the keyboard.

@cypress
Copy link

cypress bot commented Jun 29, 2023

Passing run #3343 ↗︎

0 36 0 0 Flakiness 0

Details:

Merge branch 'master' of github.com:OHIF/Viewers into remotes/origin/fix/colorma...
Project: Viewers Commit: 5cfbdc5b3e
Status: Passed Duration: 03:33 💡
Started: Jul 4, 2023 3:19 PM Ended: Jul 4, 2023 3:22 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of comments. Thanks.

extensions/cornerstone/package.json Outdated Show resolved Hide resolved
extensions/tmtv/src/commandsModule.js Outdated Show resolved Hide resolved
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.

Looks great thanks

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks. Looks like the branch needs to be updated before merge is possible.

@sedghi sedghi merged commit 64f8bb5 into master Jul 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