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: blur in io, remove panzoom and use fabricjs for panzoom #3052

Merged
merged 4 commits into from
Mar 9, 2024

Conversation

krmanik
Copy link
Contributor

@krmanik krmanik commented Mar 4, 2024

I have updated gradually so there are no atomic commits here, but I will explain what going on in this PR.

The panzoom library removed, the same functionality is implemented using
https://codepen.io/amsunny/pen/XWGLxye
http://fabricjs.com/docs/fabric.Canvas.html#viewportTransform
http://fabricjs.com/using-transformations

The IO image is set as background image for canvas, it is easier for zoom/pan using canvas only.
A bounding rectangular area added with same width and height of background image, all shapes are allowed to draw inside bounding area only.

I have used large image for testing.

output.mp4

@krmanik
Copy link
Contributor Author

krmanik commented Mar 4, 2024

The pinch to zoom is not supported in fabricjs and author of fabricjs suggested to use HammerJS in this issue #8849. I have used the library for pinch to zoom in mobile client only. Tested on device with port forwarding in chrome.

Recording.2024-03-04.221633.mp4

@dae
Copy link
Member

dae commented Mar 5, 2024

Thanks Mani. I've given this a brief test with the large image from #3037 on iOS. It works pretty well, and appears to correctly preserve the image clarity, but can get a bit laggy when zooming in/out at times, which I guess is something we can't avoid.

Would it be practical to allow the two-finger pinch gesture without requiring the user to first enter zooming mode? Interestingly on the desktop, a regular pinch to zoom shows the image in lower quality in this PR, because it's zooming the entire document. But when you zoom properly using the zoom tool, there's no image quality loss.

I've briefly skimmed the code; I'll give it a proper review in the next few days.

@krmanik
Copy link
Contributor Author

krmanik commented Mar 5, 2024

but can get a bit laggy when zooming in/out at times

I am finding solution for it.

Would it be practical to allow the two-finger pinch gesture without requiring the user to first enter zooming mode?

It is possible, I will check the implementation for it.

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.

I've looked over the code a bit more carefully. Only one minor comment:

ts/image-occlusion/mask-editor.ts Outdated Show resolved Hide resolved
krmanik added 3 commits March 9, 2024 00:36
- remove panzoom
- implement panzoom using fabricjs
- set background image for canvas
- add bounding rect for canvas
- draw or add point inside in bounding rect
- update zoom tool
@krmanik
Copy link
Contributor Author

krmanik commented Mar 8, 2024

Now, zooming can be performed in draw mode on the mobile client (panning is currently not supported). Previously, I had set the background image to the canvas, which resulted in the canvas being slow. In the current implementation, the image tag element is used instead of the background image, fixing the issues. The values are calculated based on the position of the bounding box in the canvas area and available container area.

@krmanik
Copy link
Contributor Author

krmanik commented Mar 9, 2024

Now panning is also supported. The canvas lagging, blur, zoom and pan, all issues fixed in the current PR along with support for touch based devices.

output.mp4

@dae
Copy link
Member

dae commented Mar 9, 2024

Thanks Mani, this is great - performs better, and much more ergonomic. This also improves clarify when pinch-zooming on a Mac as well. Great work!

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.

2 participants