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(scrollEvent): added out of bounds scroll #476

Conversation

Ouwen
Copy link
Contributor

@Ouwen Ouwen commented Mar 13, 2023

This adds an event for when user stack scroll goes out of bounds

@netlify
Copy link

netlify bot commented Mar 13, 2023

Deploy Preview for cornerstone-3d-docs canceled.

Name Link
🔨 Latest commit 024590b
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/6425c3249b5d1a0008fe46fa

Comment on lines 111 to 117
volumeId: volumeId,
viewport: viewport,
delta,
desiredFrameIndex,
maxFrameIndex,
currentImageId: viewport.getCurrentImageId(),
currentFrameIndex: desiredFrameIndex - delta,
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 add these to the EventTypes too?

* @param spacingInNormalDirection
* @returns { int, int }
*/
function _getDeltaFrameIndex(delta, sliceRange, spacingInNormalDirection) {
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 add the exports you want from the getVolumeViewportScrollInfo so that we don't recomute these again?

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.

see my comments

bionoone pushed a commit to bionoone/cornerstone3D-beta that referenced this pull request Mar 27, 2023
@Ouwen Ouwen force-pushed the gradienthealth/out_of_bounds_scroll_event branch from 7366080 to c8d944c Compare March 30, 2023 03:58
@Ouwen
Copy link
Contributor Author

Ouwen commented Mar 30, 2023

@sedghi this is ready for your review

@@ -59,4 +61,32 @@ export function scrollVolume(
position: newPosition,
});
viewport.render();

const { numScrollSteps, currentStepIndex } =
csUtils.getVolumeViewportScrollInfo(viewport, volumeId);
Copy link
Member

Choose a reason for hiding this comment

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

I"m a little bit confused, isn't it the fact that this function relies on the sliceRangeInfo that is calculated above? So can we refactor its logic so that we don't do sliceRangeInfo getter again?

function getVolumeViewportScrollInfo(
  viewport: IVolumeViewport,
  volumeId: string
) {
  const { sliceRange, spacingInNormalDirection } = getVolumeSliceRangeInfo(
    viewport,
    volumeId
  );

  const { min, max, current } = sliceRange;

  // Now we can see how many steps there are in this direction
  const numScrollSteps = Math.round((max - min) / spacingInNormalDirection);

  // Find out current frameIndex
  const fraction = (current - min) / (max - min);
  const floatingStepNumber = fraction * numScrollSteps;
  const currentStepIndex = Math.round(floatingStepNumber);

  return { numScrollSteps, currentStepIndex };
}

to

function getVolumeViewportScrollInfoFromSliceRange(
  sliceRange,
  spacingInNormalDirection
) {
  const { min, max, current } = sliceRange;

  // Now we can see how many steps there are in this direction
  const numScrollSteps = Math.round((max - min) / spacingInNormalDirection);

  // Find out current frameIndex
  const fraction = (current - min) / (max - min);
  const floatingStepNumber = fraction * numScrollSteps;
  const currentStepIndex = Math.round(floatingStepNumber);

  return { numScrollSteps, currentStepIndex };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sedghi let me know what you think of the change I made. I just export the sliceRangeInfo from getVolumeViewportScrollInfo

Copy link
Member

Choose a reason for hiding this comment

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

API check failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I need to build updateapi

@Ouwen Ouwen force-pushed the gradienthealth/out_of_bounds_scroll_event branch from c8d944c to 7010643 Compare March 30, 2023 15:27
@Ouwen Ouwen force-pushed the gradienthealth/out_of_bounds_scroll_event branch from 7010643 to 024590b Compare March 30, 2023 17:13
@sedghi sedghi merged commit 4cf2b63 into cornerstonejs:main Mar 30, 2023
@sedghi sedghi changed the title feat: added out of bounds scroll feat(scrollEvent): added out of bounds scroll Mar 30, 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.

2 participants