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

DICOM viewer: zoom and pan support #270

Merged
merged 6 commits into from Jan 1, 2018

Conversation

Projects
None yet
3 participants
@Serhiy-Shekhovtsov
Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 28, 2017

Enabled window width and window center adjustment. Enabled zooming and panning.
Updated the NoduleMarker component to rescale and move the marker accordingly.
Fixed bug - moving slider to cached slices didn't show those slices.

Reference to official issue

#240

Screenshot

zoom, pan

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
var scaledX = x / this.zoomRate + this.offsetX + this.markerImageSize / 2
var scaledY = y / this.zoomRate + this.offsetY + this.markerImageSize / 2
var scaledX = x
var scaledY = y

This comment has been minimized.

@louisgv

louisgv Dec 29, 2017

Contributor

If mutable, let should be preferred over var. However, I would suggest doing it like so:

const scaled = this.translation
    ? {
             x : Math.round((x + this.markerImageSize / 2 - this.translation.offsetX) / this.translation.scale),
             y : Math.round((y + this.markerImageSize / 2 - this.translation.offsetY) / this.translation.scale)
        } 
    : { x, y }
         
    EventBus.$emit('nodule-marker-moved', scaled.x, scaled.y, this.sliceIndex)

This comment has been minimized.

@Serhiy-Shekhovtsov
@@ -1,20 +1,22 @@
<template>
<div class="DICOM-container" style="width: 100%">

This comment has been minimized.

@lamby

lamby Dec 29, 2017

Contributor

Please don't make unrelated changes in commits - when you use the word "and" or a command (",") in a commit message, it often means that you should split it up into separate commits - makes reviewing and discussion so much easier! (eg. why remove this style? you could explain here but "nobody" will look or can find at this review comment later alas..)

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Dec 29, 2017

Contributor

@lamby noted! Will update my PR. Btw, this change is a part of fixing the bug with misinterpretation of the predicted coordinates.

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Dec 29, 2017

Contributor

Moved to separate commit.

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/dicom-viewer-zoom-and-pan branch from b2eecb2 to b17c0be Dec 29, 2017

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/dicom-viewer-zoom-and-pan branch from b17c0be to 87a6d1a Dec 29, 2017

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 29, 2017

Looks like the build has been failing since PR #267.

@lamby lamby merged commit 843019e into drivendataorg:master Jan 1, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
concept-to-clinic/cla @Serhiy-Shekhovtsov has signed the CLA.
Details
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Jan 1, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment