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

feat: added MIP option to StackScrollTool #537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ouwen
Copy link
Contributor

@Ouwen Ouwen commented Apr 1, 2023

No description provided.

@netlify
Copy link

netlify bot commented Apr 1, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 76a2109
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/642866fdd7f9360008e09df3
😎 Deploy Preview https://deploy-preview-537--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 settings.

@Ouwen Ouwen force-pushed the gradienthealth/added_mip_option branch from 4f7356a to 6b72360 Compare April 1, 2023 17:02
@Ouwen Ouwen force-pushed the gradienthealth/added_mip_option branch from 6b72360 to 76a2109 Compare April 1, 2023 17:16
@Ouwen
Copy link
Contributor Author

Ouwen commented Apr 1, 2023

@sedghi, this PR is ready for your review!

constructor(
toolProps: PublicToolProps = {},
defaultToolProps: ToolProps = {
supportedInteractionTypes: ['Mouse', 'Touch'],
configuration: {
invert: false,
leftRightMode: false,
Copy link
Member

@sedghi sedghi Apr 5, 2023

Choose a reason for hiding this comment

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

what is the difference between leftRightMode and mipMode? can leftRightMode be not mipMode? to me this configuration is complex and not clear at the first glance. there are multiple levels that has enabled, how about something like

configuration: {
  activeMode: 'stackScrollMode', // or 'mipMode'
  stackScrollMode: {
    invert: false,
    loop: false,
    debounceIfNotLoaded: true,
  },
  mipMode: {
    pixelsPerThickness: 5,
    minSlabThickness: 5e-2,
    maxSlabThickness: 30,
  },
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the following options need some way to be selected...

leftRight vs upDown
Inversion is rightLeft vs downUp
Then MIP can be active/inactive
StackScroll can be active/inactive

Both MIP and StackScroll can be active at the same time.

What about something like this:

configuration: {
  direction: {
   stackScrollMode: enums['upDown', 'downUp', null]
   mipMode: enums['leftRight', 'rightLeft', null]
  },
  stackScrollMode: {
    loop: false,
    debounceIfNotLoaded: true,
  },
  mipMode: {
    pixelsPerThickness: 5,
    minSlabThickness: 5e-2,
    maxSlabThickness: 30,
  },
},

Copy link
Member

Choose a reason for hiding this comment

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

how about we stick to horizontal and vertical and assume (naturally) that right is more and up is more, and we have a invert flag?

configuration: {
  stackScrollMode: {
    enabled: true,
	invert: true, 
    direction: 'vertical'
    loop: false,
    debounceIfNotLoaded: true,
  },
  mipMode: {
    enabled: false, 
    invert: true, 
    direction: 'horizontal'
    pixelsPerThickness: 5,
    minSlabThickness: 5e-2,
    maxSlabThickness: 30,
  },
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works for me!

Comment on lines +200 to +207
leftRightMode: false,
mipMode: {
enabled: true,
invert: false,
pixelsPerThickness: 5,
minSlabThickness: 5e-2,
maxSlabThickness: 30,
},
Copy link
Member

Choose a reason for hiding this comment

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

I guess this example should get updated too right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep still need to push the changes

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.

2 participants