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: remove unnecessary event firing for annotations #123

Merged
merged 10 commits into from
Jun 14, 2022
Merged

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Jun 7, 2022

Fix:

  • ANNOTATION_RENDER events were firing even if no annotation were rendered
  • Camera API to be consistent between CPU and GPU rendering routes

Feat:

  • Add new suppressEvents to StackViewport setProperties

@sedghi sedghi requested review from swederik and JamesAPetts June 7, 2022 13:29
Copy link
Member

@JamesAPetts JamesAPetts left a comment

Choose a reason for hiding this comment

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

Minor comments to change but mostly LGTM.

packages/core/src/RenderingEngine/StackViewport.ts Outdated Show resolved Hide resolved
packages/tools/src/tools/CrosshairsTool.ts Outdated Show resolved Hide resolved
packages/tools/src/tools/ZoomTool.ts Outdated Show resolved Hide resolved
@sedghi sedghi requested a review from JamesAPetts June 9, 2022 20:39
Copy link
Member

@JamesAPetts JamesAPetts left a comment

Choose a reason for hiding this comment

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

Looks like the camera stuff is almost there, but looks like it will break on rotation.

packages/core/src/RenderingEngine/StackViewport.ts Outdated Show resolved Hide resolved
packages/core/src/RenderingEngine/StackViewport.ts Outdated Show resolved Hide resolved
// half of the height of the viewport in the world unit (mm), we can use that
// to compute the scale factor which is the ratio of the viewport height in pixels
// to the current rendered image height.
const { rowPixelSpacing } = image;
Copy link
Member

Choose a reason for hiding this comment

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

Ooooh, what if the viewport is rotated? I don't think it correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems to work fine


viewport.scale += diff; // parallelScale; //viewport.scale < 0.1 ? 0.1 : viewport.scale;
if (scale) {
const { rowPixelSpacing } = image;
Copy link
Member

Choose a reason for hiding this comment

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

And here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems to work fine


import _getHash from './_getHash';

import _setAttributesIfNecessary from './_setAttributesIfNecessary';
import _setNewAttributesIfValid from './_setNewAttributesIfValid';

function drawCircle(
svgDrawingHelper: any,
svgDrawingHelper: SVGDrawingHelper,
Copy link
Member

Choose a reason for hiding this comment

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

I thought we settled on svgDraingHelpers?

@sedghi sedghi requested a review from JamesAPetts June 13, 2022 19:04
Copy link
Member

@JamesAPetts JamesAPetts left a comment

Choose a reason for hiding this comment

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

LGTM

@sedghi sedghi merged commit 03551d9 into main Jun 14, 2022
@swederik swederik deleted the feat/events-improv branch July 8, 2022 09:32
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.

3 participants