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

fix: 1054 perpendicular bidirectional tool on non-square pixel images #1063

Merged
merged 27 commits into from
Oct 9, 2019

Conversation

brunoalvesdefaria
Copy link
Collaborator

@brunoalvesdefaria brunoalvesdefaria commented Sep 10, 2019

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

This PR fixes the issue #1054 where the bidirectional tool was not being created with its lines perpendicular for images with non-square pixels.
The angle between its lines were always assuming an angle different from 90º.
The issue was happening both when the users were creating a bidirectional tool measurement or editing an existing one.

With this fix, most part of the complexity and duplicated code was removed from the logic responsible to calculate the handles position when creating or editing bidirectional tool measurements.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #1063 into master will increase coverage by 1.17%.
The diff coverage is 95.74%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1063      +/-   ##
=========================================
+ Coverage   17.83%     19%   +1.17%     
=========================================
  Files         274     282       +8     
  Lines        8428    8391      -37     
  Branches     1420    1419       -1     
=========================================
+ Hits         1503    1595      +92     
+ Misses       5759    5643     -116     
+ Partials     1166    1153      -13
Impacted Files Coverage Δ
...bidirectionalTool/moveHandle/setHandlesPosition.js 0% <0%> (ø) ⬆️
...moveHandle/moveLongLine/updatePerpendicularLine.js 100% <100%> (ø)
...dle/movePerpendicularLine/movePerpendicularLine.js 100% <100%> (ø)
...nnotation/bidirectionalTool/utils/getLineVector.js 100% <100%> (ø)
.../moveHandle/movePerpendicularLine/getHelperLine.js 100% <100%> (ø)
...e/movePerpendicularLine/updatePerpendicularLine.js 100% <100%> (ø)
.../moveHandle/movePerpendicularLine/lineHasLength.js 100% <100%> (ø)
...tation/bidirectionalTool/moveHandle/getBaseData.js 100% <100%> (ø)
...rectionalTool/utils/getDistanceWithPixelSpacing.js 100% <100%> (ø)
...tionalTool/utils/updatePerpendicularLineHandles.js 82.6% <100%> (+82.6%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f5e46c...55a5388. Read the comment docs.

@dannyrb
Copy link
Member

dannyrb commented Sep 10, 2019

@brunoalvesdefaria, please feel free to ping me if you have questions/pushback.

Copy link
Member

@dannyrb dannyrb left a comment

Choose a reason for hiding this comment

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

  • test('') and it('') should read as plain english
  • centralize updated by reference to the tool itself
    • If you have alternative thoughts/suggestions, we can discuss
  • Prefer object equality assertions over many individual property assertions
  • Use toBeCloseTo for float equality checks
  • A function/method should only take the parameters it requires in order to function. Passing baseData in it's entirety can make it confusing/difficult to know what data is actually required for the function to work -- it also makes re-use much more difficult
  • updateSomeData functions should be getUpdatedSomeData, where you return the new and set it in the tool where you can isolate changes to the reference.

@dannyrb dannyrb merged commit e7eaa37 into master Oct 9, 2019
@dannyrb
Copy link
Member

dannyrb commented Oct 9, 2019

🎉 This PR is included in version 4.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@brunoalvesdefaria brunoalvesdefaria deleted the issue-1054 branch October 11, 2019 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants