Skip to content

Volume quality slider - #283

Merged
floryst merged 14 commits into
Kitware:mainfrom
sankhesh:volume_quality_slider
Apr 19, 2023
Merged

Volume quality slider#283
floryst merged 14 commits into
Kitware:mainfrom
sankhesh:volume_quality_slider

Conversation

@sankhesh

@sankheshsankhesh commented Feb 14, 2023

Copy link
Copy Markdown
Collaborator

Fixes#277 and #248

@netlify

netlifyBot commented Feb 14, 2023

Copy link
Copy Markdown

Deploy Preview for volview ready!

NameLink
🔨 Latest commite5d11d5
🔍 Latest deploy loghttps://app.netlify.com/sites/volview/deploys/63eb9ed6df518a0008d25ec8
😎 Deploy Previewhttps://deploy-preview-283--volview.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 settings.

@sankhesh
sankheshforce-pushed the volume_quality_slider branch from 6bb070f to f796fb4CompareFebruary 14, 2023 14:44
@sankhesh

Copy link
Copy Markdown
CollaboratorAuthor

@aylward PTAL

@sankhesh
sankheshforce-pushed the volume_quality_slider branch from f796fb4 to e5d11d5CompareFebruary 14, 2023 14:46
@aylward

Copy link
Copy Markdown
Contributor

Looks great! The slider has a noticeable impact - but is there a way to increase realism even further when the maximum quality is selected? On my old system, it is still quite fast on the maximum quality setting - and at that setting we can definitely take more time to do renderings if the quality can be improved.

Comment threadsrc/components/VtkThreeView.vue
Comment threadsrc/components/VolumeProperties.vue Outdated
@florystfloryst linked an issue Feb 22, 2023 that may be closed by this pull request
Comment threadsrc/store/view-configs/volume-coloring.ts Outdated
Comment threadsrc/components/VtkThreeView.vue
@sankhesh
sankheshforce-pushed the volume_quality_slider branch from e5d11d5 to f112453CompareApril 16, 2023 04:33
@netlify

netlifyBot commented Apr 16, 2023

Copy link
Copy Markdown

Deploy Preview for volview-dev ready!

NameLink
🔨 Latest commit8b0900f
🔍 Latest deploy loghttps://app.netlify.com/sites/volview-dev/deploys/643eb817edcd0100086c7d06
😎 Deploy Previewhttps://deploy-preview-283--volview-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 settings.

@aylward

Copy link
Copy Markdown
Contributor

Absolutely beautiful! And stable on my phone!

Screenshot_20230416-071951

@sankhesh

Copy link
Copy Markdown
CollaboratorAuthor

@aylward What do you think? -

image

@sankheshsankhesh mentioned this pull request Apr 17, 2023
@sankhesh
sankheshforce-pushed the volume_quality_slider branch from 8cf0f63 to ef58654CompareApril 17, 2023 15:57
@aylward

Copy link
Copy Markdown
Contributor

Thinking about this - I don't think the quality is ever "low"

How about
Good - Better - Ultra - Experimental

One issue also is that there isn't much (enough) space between Ultra and Experimental in the about figure - looks like one label "Ultra Experimental"

Perhaps change Experimental to Beta?

Good - Better - Ultra - Beta

?

@sankhesh
sankheshforce-pushed the volume_quality_slider branch from ef58654 to 9c07a31CompareApril 17, 2023 22:47
@sankhesh

Copy link
Copy Markdown
CollaboratorAuthor

@aylward Good to merge?

@aylwardaylward left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great!!!!

Perhaps the "Don't show again" button could be shown in a different color or have a stronger outline around the button? It is a little hard to tell it is a button on my display.

Comment threadsrc/components/VolumeProperties.vue Outdated
Comment threadsrc/components/VolumeProperties.vue Outdated
@sankhesh
sankheshforce-pushed the volume_quality_slider branch from 0455edb to 35a0502CompareApril 18, 2023 15:20
@sankhesh
sankheshforce-pushed the volume_quality_slider branch from 20782e4 to 8b0900fCompareApril 18, 2023 15:32
Comment on lines +56 to +57
const lightingModel = ref<keyof typeof LIGHTING_MODELS>('hybrid');
const selectLightingMode = (buttonTxt: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here is a stronger and more succinct typing for selectLightingMode:

Suggested change
const lightingModel = ref<keyof typeof LIGHTING_MODELS>('hybrid');
const selectLightingMode = (buttonTxt: string) => {
type LightingModel = keyof typeof LIGHTING_MODELS;
const lightingModel = ref<LightingModel>('hybrid');
const selectLightingMode = (buttonTxt: LightingModel) => {

@floryst

Copy link
Copy Markdown
Contributor

Here is an idea I think would be good to have, but we can do this after this PR is merged: we can have a question mark icon that, when clicked, goes to a page in our documentation that explains what it means when we way "Ultra / Beta is unstable on some systems".

@aylward

aylward commented Apr 18, 2023 via email

Copy link
Copy Markdown
Contributor

@sankhesh
sankhesh added this pull request to the merge queueApr 19, 2023
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to no response for status checks Apr 19, 2023
@sankhesh

Copy link
Copy Markdown
CollaboratorAuthor

@floryst@aylward How do I bypass the merge queue? I just a see a button Merge when ready and it keeps waiting for the two jobs that don't have anything to do.

@floryst

Copy link
Copy Markdown
Contributor

I just enabled merge queues in VolView to trial this feature. I think it's not fully set up yet. I'll handle it.

@floryst
floryst added this pull request to the merge queueApr 19, 2023
Merged via the queue into Kitware:main with commit 71d69c5Apr 19, 2023
PaulHax pushed a commit to PaulHax/VolView that referenced this pull request Apr 24, 2025
Sign up for freeto 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.

3D sample distance slider

4 participants

@sankhesh@aylward@floryst@PaulHax