Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

DICOM viewer: zoom and pan support #270

Conversation

Serhiy-Shekhovtsov
Copy link
Contributor

@Serhiy-Shekhovtsov 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

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

Choose a reason for hiding this comment

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

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..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to separate commit.

@Serhiy-Shekhovtsov
Copy link
Contributor Author

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
@lamby
Copy link
Contributor

lamby commented Jan 1, 2018

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants