Feature/1778 Path cropping #1873

zero41120 · 2020-08-15T03:46:05Z

Issue: #1778
Implemented a cropping logic for path using canvas transformation.

Above Here is the original polygon cropping feature.

This is the new path cropping feature

The example rectangular shape in polygon and path has the same dimension,
showing that the new path cropping crops correct location.

This is working in progress, currently the crop location and size are incorrect, but cropped region remains the same when zooming.

My lack of understanding of sketch canvas causes the cropping path not translated correctly. My assumption is that if we can idsable it on an devices like iOS, then we should be able to use it without the sketch canvas

User can optionally set the cropWithoutSketch to false to use the sketch canvas when needed.

zero41120 · 2021-03-09T06:48:21Z

@iangilman In our enterprise software, we have been using this feature for long time and didn't notice any major issue.
Would you take look and see if something is missing?

iangilman

Apologies for taking so long to get to this! I really fell off of the OSD track in 2020. I'm trying to get back to it, though!

This looks like a great new feature... good to be able to add curves in to our cropping! I have a number of comments/questions, but hopefully we can work through them all if you're still up for it :) Thank you for contributing this!

iangilman · 2021-03-17T18:57:54Z

src/tiledimage.js

+
+    /**
+     * This function converts the top left viewport coordinates to pixels coordinates
+     * @param {Boolean} current Pass true for the current location; defaults to false (target location).


We need a documentation line for withPixelDensityRatio.

iangilman · 2021-03-17T18:59:57Z

src/tiledimage.js

@@ -1883,6 +1939,56 @@ function compareTiles( previousBest, tile ) {
    return previousBest;
 }

+/**
+ * This function checks if the path cropping is ready.


This function should be marked as private with @private.

I think it would be helpful to add a little more documentation about what this function does and what its purpose is.

iangilman · 2021-03-17T20:49:27Z

src/tiledimage.js

+}
+
+/**
+ * This function crops the view with tiledImage._croppingPaths by transforming


Please mark this as @private and document the useSketch argument.

iangilman · 2021-03-31T18:39:26Z

src/drawer.js

@@ -201,6 +201,37 @@ $.Drawer.prototype = {
        context.clip();
    },

+    /**
+     * This function will create multiple polygon paths on the drawing context by provided polygons,


We should get clear with our terminology. We already have clipping by polygon, and now we are adding clipping by path. In the doc comment here, we shouldn't refer to the paths as polygons.

iangilman · 2021-03-31T18:41:05Z

src/drawer.js

+    /**
+     * This function will create multiple polygon paths on the drawing context by provided polygons,
+     * then clip the context to the paths.
+     * @param {Path2D} - path objects to clip with.


OpenSeadragon supports IE11, which does not support Path2D. We should add something to the doc comments for this feature to make it clear that it's up to the developer to come up with an alternate strategy for IE11 if they support that browser.

iangilman · 2021-03-31T18:55:26Z

src/tiledimage.js

+     * var pathData = "M 500,500 h 200 v 200 h -200 z";
+     * var pathObj = document.createElementNS("http://www.w3.org/2000/svg", "path");
+     * pathObj.setAttribute('d', pathData);
+     * @param cropWithoutSketch - optional - indicate that cropping path should not


What's the purpose of this? Seems like an implementation detail we wouldn't necessarily want to expose.

iangilman · 2021-03-31T18:57:44Z

src/tiledimage.js

@@ -678,6 +678,38 @@ $.extend($.TiledImage.prototype, $.EventSource.prototype, /** @lends OpenSeadrag
        this._setScale(height / this.normHeight, immediately);
    },

+    /**
+     * Sets an array of paths to crop the TiledImage during draw tiles.


I figure this function is going to be where people are most likely to look to learn about this feature, so this is the place to do some extra documentation, including the fact that this feature doesn't work on IE11, and the fact that you can't crop with both paths and polygons.

On that latter point, setCroppingPolygons should be updated to include a doc comment on the same point.

iangilman · 2021-03-31T18:58:25Z

src/tiledimage.js

+     * pathObj.setAttribute('d', pathData);
+     * @param cropWithoutSketch - optional - indicate that cropping path should not
+     * use the sketch canvas default to true.
+     * @param preventSketchTransform - optional - indicate that the cropping path


What's the purpose of this?

iangilman · 2021-03-31T20:52:22Z

src/tiledimage.js

+     * @param {Boolean} current Pass true for the current location; defaults to false (target location).
+     * @returns {OpenSeadragon.Point}
+     */
+    getOriginPixelCoordinate: function(current, withPixelDensityRatio) {


We should think about whether we need this as part of the public API. My initial thought is that it should be private.

Either way, it needs a better name, as "pixel" isn't specific enough. Maybe getOriginDrawerCoordinate.

iangilman · 2021-03-31T20:55:11Z

src/tiledimage.js

        tiledImage.getRotation(true) % 360 === 0 && // TODO: support tile edge smoothing with tiled image rotation.
        $.supportsCanvas) {
        // When zoomed in a lot (>100%) the tile edges are visible.
        // So we have to composite them at ~100% and scale them up together.
        // Note: Disabled on iOS devices per default as it causes a native crash
+        // Node: Disabled when _croppingPaths are provided because stacking translation


Typo: I assume "node" should be "note".

zero41120 · 2021-04-01T16:56:39Z

Thanks for the feedback/comments.
I'm a bit busy recently. I will get back to this pull request once I have some free time.

iangilman · 2021-04-02T18:51:20Z

@zero41120 great! No rush (clearly it took me a while to get back to you), but when you can, it would be lovely :)

iangilman · 2021-07-07T23:22:50Z

src/tiledimage.js

+    try {
+        var context = tiledImage._drawer._getContext(useSketch);
+        var viewport  = tiledImage._drawer.viewport;
+        var oldMatrix = context.getTransform();


As noted in #1912, this won't work on IE 11, which we still support. That said, IE 11 also doesn't support Path2D, so it's actually fine to use in this context (in other words, you won't be using this feature at all on IE 11). We just need to make sure this code doesn't get hit when not using this feature (which certainly seems to be the case).

zero41120 added 12 commits August 4, 2020 18:41

1778 Setup path cropping page

824591a

1778 Add set and reset of path data

428254d

1778 Crop provided path directly without transformation

69d9b57

1778 Add suggested transformation which doesn't work

34190a3

1778 Validate condition before cropping with path

6b7d1db

1778 Add logic to transfrom context before cropping

47e0c8b

This is working in progress, currently the crop location and size are incorrect, but cropped region remains the same when zooming.

1778 Scale canvas for cropping path

133e178

1778 Translate canvas for cropping path

43e5805

1778 Remove extra rotate

744b0c9

1778 Rename cropping buttons' id

1347e0c

1778 Add rotate buttons to demo page

b784316

1778 Adjust example polygon and path to draw at same location

5c9a963

zero41120 mentioned this pull request Aug 15, 2020

Path Data cropping support #1778

Open

zero41120 and others added 9 commits August 14, 2020 20:59

1778 Add documentations

650624a

1778 Remove Sketch canvas if using cropping path

3013a17

My lack of understanding of sketch canvas causes the cropping path not translated correctly. My assumption is that if we can idsable it on an devices like iOS, then we should be able to use it without the sketch canvas

1778 Add cropping polygon support for multi image

51407eb

1778 Add multi image cropping support to demo page

43f5083

1778 Update docs

9841016

1778 Use sketch canvase when cropping with path

d797158

User can optionally set the cropWithoutSketch to false to use the sketch canvas when needed.

1778 Add more control option for sketch canvas with cropping path

82efeed

1778 Allow use to provide fill rule for path clipping

47ddfe5

1778 Allow use to provide fill rule for path clipping

004e640

zero41120 mentioned this pull request Nov 25, 2020

Tracking transformation Matrix for IE support #1912

Open

iangilman requested changes Mar 31, 2021

View reviewed changes

iangilman reviewed Jul 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1778 Path cropping #1873

Feature/1778 Path cropping #1873

zero41120 commented Aug 15, 2020

zero41120 commented Mar 9, 2021

iangilman left a comment

iangilman Mar 17, 2021

iangilman Mar 17, 2021

iangilman Mar 17, 2021

iangilman Mar 17, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

iangilman Mar 31, 2021

zero41120 commented Apr 1, 2021

iangilman commented Apr 2, 2021

iangilman Jul 7, 2021

Feature/1778 Path cropping #1873

Are you sure you want to change the base?

Feature/1778 Path cropping #1873

Conversation

zero41120 commented Aug 15, 2020

zero41120 commented Mar 9, 2021

iangilman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero41120 commented Apr 1, 2021

iangilman commented Apr 2, 2021

Choose a reason for hiding this comment