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.

Sign up for GitHub

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

Jump to bottom

fix problem with alligned overlay #6445

Merged
merged 1 commit into from Nov 14, 2023
Merged

fix problem with alligned overlay #6445

merged 1 commit into from Nov 14, 2023

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Nov 13, 2023

Description

During implementation of #6438 I wrongly assume that the transform box updates layer transform properties. But it update layer.affine.transform, not layer.transform

So it lead to bug that could be seen on this video:

napari_overlays-2023-11-13_17.35.54.mp4
napari_overlays-2023-11-13_17.39.22.mp4

This PR fixes this problem.

@Czaki Czaki added the bugfix PR with bugfix label Nov 13, 2023
@Czaki Czaki added this to the 0.4.19 milestone Nov 13, 2023
brisvag
brisvag approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Yup, we all missed this, oops ^^' There's a larger discussion to be had about this affine vs composite affine (scale + shear + rotation + translation), but let's do that elsewhere. This fixes the bug and retains current behaviour.

@Czaki Czaki added the ready to merge Last chance for comments! Will be merged in ~24h label Nov 13, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #6445 (6c2a300) into main (932054d) will decrease coverage by 0.07%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6445      +/-   ##
==========================================
- Coverage   92.19%   92.13%   -0.07%     
==========================================
  Files         599      599              
  Lines       52987    52989       +2     
==========================================
- Hits        48853    48820      -33     
- Misses       4134     4169      +35
Files Coverage Δ
napari/_qt/qt_viewer.py 76.74% <100.00%> (+0.10%) ⬆️
napari/_vispy/layers/base.py 98.49% <ø> (ø)

... and 11 files with indirect coverage changes

@Carreau
Copy link
Contributor

Carreau commented Nov 14, 2023

This got the "Ready-to-merge" label 24h+ ago and got no objections. Merging to move the queue forward.

Thanks.

@Carreau Carreau merged commit c9c3981 into napari:main Nov 14, 2023
35 checks passed
@Carreau
Copy link
Contributor

Carreau commented Nov 14, 2023

I'm leaving the "ready to merge" because it needs backport on 0.4.19

@Czaki Czaki deleted the bugfix/overalys_alignment branch November 14, 2023 19:20
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Nov 14, 2023
Czaki added a commit that referenced this pull request Nov 14, 2023
@Czaki
Fix problem with aligned overlay (#6445)
4b00f7b 
During implementation of #6438 I wrongly assume that the transform box
updates layer `transform` properties. But it update
`layer.affine.transform`, not `layer.transform`

So it lead to bug that could be seen on the video in #6445
Czaki added a commit that referenced this pull request Nov 24, 2023
@Czaki
Fix problem with aligned overlay (#6445)
734208a 
During implementation of #6438 I wrongly assume that the transform box
updates layer `transform` properties. But it update
`layer.affine.transform`, not `layer.transform`

So it lead to bug that could be seen on the video in #6445
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brisvag brisvag brisvag approved these changes

Assignees
No one assigned
Labels
bugfix PR with bugfix
Projects
None yet
Milestone
0.4.19
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Czaki @Carreau @brisvag