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 occlusion drift again #3443

Merged
merged 10 commits into from
Oct 2, 2024
Merged

Conversation

taylorobyen
Copy link
Contributor

Fixes #3370

Addressed bugs preventing occlusions from being added as a result of my previous fix.

I'm opening this as a draft for now, because I'm getting some strange behavior with the occlusions appearing on images they aren't supposed to. I'm able to recreate this on the main branch as well as the latest official version of Anki, so it doesn't appear to be my changes causing it. I will follow up once I have more information.

@dae
Copy link
Member

dae commented Sep 27, 2024

Ouch, that's no good. If it's reproducible, would you mind bisecting to figure out the commit that caused it, and providing steps to reproduce?

@dae dae added this to the 24.10 milestone Sep 27, 2024
@taylorobyen
Copy link
Contributor Author

taylorobyen commented Sep 28, 2024

Latest 2 commits fix #3449, upon testing I don't see any new regressions/bugs as a result of my changes. The entire architecture was a bit of a mess, so I refactored it a bit to be more streamlined. It seems like the issue was caused from $: $changeSignal, onChange(); in MaskEditor.svelete being called immediately upon note change which would overwrite the selected notes shapes with the previously selected notes shapes.

I have also found a few more unrelated bugs that I will report. Once I knock out these next couple bugs I'll see if I can identify when #3449 was introduced.

Edit: Issues of said bugs: #3451, #3452, #3456

@taylorobyen
Copy link
Contributor Author

Fixed #3451, #3452, and #3456. Also, I made it so on creation of text labels it enters edit mode on the label and switches the tool back to the cursor. I found having to click onto the label to edit it after creation tedious and then clicking off the label to deselect I would always unintentionally create additional labels.

I've done some more testing and now everything seems to work as intended.

@taylorobyen taylorobyen marked this pull request as ready for review September 28, 2024 20:40
Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Thanks for looking into these issues Taylor!

@krmanik I know you're busy recently, but I wanted to give you a brief chance to review this PR, as it's mainly adjusting code I think you wrote in the past. If I don't hear from you in 48 hours, I'll merge this in, assuming there are no other pending changes.

ts/routes/image-occlusion/store.ts Outdated Show resolved Hide resolved
@dae
Copy link
Member

dae commented Oct 2, 2024

Thanks Taylor! It's a bit earlier than 48 hours, but I presume Mani doesn't have anything to add.

@dae dae merged commit d6aa959 into ankitects:main Oct 2, 2024
1 check passed
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.

Image occlusion masks don't stay in the same place
2 participants