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

DSEGOG-128 Image centroids/crosshairs #521

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

louise-davies
Copy link
Member

@louise-davies louise-davies commented Nov 28, 2024

Description

Adds the ability to go into "crosshairs" mode. When switching into this mode, it will display the centroid for the image and the corresponding intensity plots. The user can click on the image to reposition the crosshairs. Resetting the view resets back to the centroid.

It should work when switching via the thumbnail selector, we always default to the centroid though.

image

Testing instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check Actions build
  • Review changes to test coverage
  • Play around with the crosshairs mode

Agile board tracking

Closes DSEGOG-128

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 97.04017% with 14 lines in your changes missing coverage. Please review.

Project coverage is 97.86%. Comparing base (4e7c5a3) to head (2545610).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/images/imageView.component.tsx 89.70% 14 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #521      +/-   ##
===========================================
- Coverage    97.90%   97.86%   -0.05%     
===========================================
  Files           93       94       +1     
  Lines        12108    12532     +424     
  Branches      1974     2024      +50     
===========================================
+ Hits         11854    12264     +410     
- Misses         250      264      +14     
  Partials         4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@louise-davies louise-davies marked this pull request as ready for review December 5, 2024 14:56
joshuadkitenge
joshuadkitenge previously approved these changes Dec 12, 2024
Copy link
Contributor

@joshuadkitenge joshuadkitenge left a comment

Choose a reason for hiding this comment

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

LGTM, work well just one question.

Comment on lines 123 to 132
width: imageElement?.naturalWidth
? imageElement.naturalWidth + 29
: 200,
height: 200,
}
: {
width: 200,
height: imageElement?.naturalHeight
? imageElement.naturalHeight + 22
: 200,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the natural width and height need 29 and 22 added ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I should add a comment for this, but I needed to add some padding to account for the width/height of the Chart.js axis ticks (aka the numbers) so that the actual axis line itself aligns with the image.

Maybe I can try and calculate it based on the default config variables in Chart.js - but they may just have to remain magic numbers...

I will at least extract both out to variables at the top of the file (to make them easy to edit if we need) and explain what they're for

@joshuadkitenge
Copy link
Contributor

Should an exported images also include the crosshairs ??

@louise-davies
Copy link
Member Author

@joshuadkitenge no it shouldn't (I think - they're not exported in eCat) - if they do it's an oversight on my part (although I wouldn't think they would as I would have thought we download the raw image itself, not the canvas overlay where the crosshairs are drawn...)

- this helps reduce blurriness when moving between displays with different devicePixelRatio
…re reliable after the responsive: true change
- the calculation stuff was unreliable across browsers, instead just use the constants we know currently work
and rely on our snapshot testing to highlight any issues if defaults change in the future
@louise-davies louise-davies added the WIP This PR is not finished yet label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP This PR is not finished yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants