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: modifying context of canvas for rendering one tool affects rendering other tools #1315

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

codepage949
Copy link
Contributor

fixes #1311

  • 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)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

fixes bug

  • What is the current behavior? (You can also link to an open issue here)

before

  • What is the new behavior (if this is a feature change)?

after

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

no

  • Other information:

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #1315 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
- Coverage   19.01%   19.00%   -0.01%     
==========================================
  Files         286      286              
  Lines        8652     8655       +3     
  Branches     1479     1479              
==========================================
  Hits         1645     1645              
- Misses       5801     5804       +3     
  Partials     1206     1206              
Impacted Files Coverage Δ
...c/eventDispatchers/imageRenderedEventDispatcher.js 20.00% <0.00%> (-3.53%) ⬇️

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 79660e9...9827865. Read the comment docs.

@codepage949 codepage949 changed the title fix: saving and restoring canvas' context before rendering in onImageRendered #1311 fix: saving and restoring canvas' context before rendering in onImageRendered Sep 24, 2020
@codepage949 codepage949 changed the title fix: saving and restoring canvas' context before rendering in onImageRendered fix: modifying context of canvas for rendering one tool affects rendering other tools Oct 12, 2020
@codepage949
Copy link
Contributor Author

@dannyrb

if you checked this pr and had some problem, please let me know what the problem is.

i think this bug is critical so i hope to fix as soon as possible.

@dannyrb
Copy link
Member

dannyrb commented Oct 12, 2020

This looks straightforward. Is there a specific tool you're having issues with? Most should be independently using save/restore to prevent this issue.

I can see where this protects from 3rd party tools and any other tools that don't save/restore.

@codepage949
Copy link
Contributor Author

@dannyrb

below link describes the issue in more detail.

#1311 (comment)

@dannyrb
Copy link
Member

dannyrb commented Oct 13, 2020

Thanks for taking the time to troubleshoot and PR this. As well as @mentioning me. I know I can be a bit hard to get a hold of these days.

I think your solution makes sense, and I can't see how it could hurt. If anything, it should reduce the number of situations in which tools need to use this pattern themselves. I think the draw API wraps itself with this logic already?

Anywhoo. If this doesn't resolve your issues, or if you notice any odd side-effects, please ping me.

@dannyrb dannyrb merged commit 3925399 into cornerstonejs:master Oct 13, 2020
@dannyrb
Copy link
Member

dannyrb commented Oct 13, 2020

🎉 This PR is included in version 4.22.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

[bug] problems occur when using overlays and annotations at the same time
2 participants