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(Canvas): flipped viewport transform coords #7515

Merged
merged 7 commits into from Jun 12, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Nov 23, 2021

Motivation

#7471

gist

When flipped (negative scale) Object.isOnScreen breaks because it tried to check if a point is within vptCoords which always returned false because when flipped tl was greater than br.

Important

I've adjusted tests.
I need you to review that they are good enough, viewport transform is confusing.
The test enhancement exposed another bug with isPartiallyOnScreen.
It's test used stale canvas state, that is why it passed. Now it fails.
The reason it fails is because the rect has a common line with canvas vpt so the intersection test running in isPartiallyOnScreen returns true. But it is wrong. isPartiallyOnScreen should return false in this case, which is unhandled.

@ShaMan123 ShaMan123 mentioned this pull request Nov 27, 2021
9 tasks
@stale
Copy link

stale bot commented Dec 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Dec 14, 2021
@ShaMan123 ShaMan123 removed the stale Issue marked as stale by the stale bot label Dec 14, 2021
@stale
Copy link

stale bot commented Dec 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Dec 28, 2021
@stale stale bot closed this Jan 6, 2022
@ShaMan123 ShaMan123 reopened this Jan 6, 2022
@stale stale bot removed the stale Issue marked as stale by the stale bot label Jan 6, 2022
@asturur
Copy link
Member

asturur commented Jan 9, 2022

Didn't we fix this already?
Ok this is different.

@asturur asturur added the feature label Jan 9, 2022
@asturur
Copy link
Member

asturur commented Jan 9, 2022

@ShaMan123 if you have time to move this PR in a branch i can look into test failures and finish it.

@ShaMan123 ShaMan123 self-assigned this Jan 9, 2022
@ShaMan123
Copy link
Contributor Author

@ShaMan123 if you have time to move this PR in a branch i can look into test failures and finish it.

@asturur ?

asturur
asturur previously approved these changes Jun 12, 2022
@ShaMan123
Copy link
Contributor Author

Should we rebase? Update from master? I'll leave it to you.
I haven't visited this PR in a long time

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jun 12, 2022

Should we rebase? Update from master? I'll leave it to you.

Cause I'm afraid of unwanted overwrite

@asturur
Copy link
Member

asturur commented Jun 12, 2022

one test needed to be updated, since it had a flipped viewport, and so the new function was supposed to change the test result.
The other i don't know but was effectively an edge case, so i just made less edgy.

@asturur asturur merged commit 27e89ce into fabricjs:master Jun 12, 2022
@ShaMan123 ShaMan123 deleted the fix-viewportTransform-isOnScreen branch September 12, 2022 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants