adding setting of globalCompositeOperation for tiledImage #814

meihuisu · 2016-01-06T18:27:03Z

globalCompositeOperation for tag is used to allow
multiple tiledImage be blended in a specific way.
(special handling,
when compositeOperation is 'source-over' and opacity is 1,
useSketch is false, otherwise useSketch is true
)
Valid values are 'source-atop', 'source-in', 'source-out',
'destination-over', 'destination-atop', 'destination-in',
'destination-out', 'lighter', 'copy' or 'xor'
http://www.w3schools.com/tags/canvas_globalcompositeoperation.asp

html <canvas> is used to process multiple tiledImages to blend is a specific way. (special handling, when compositeOperation is 'source-over' and opacity is 1, useSketch is false, otherwise useSketch is true ) Valid values are 'source-atop', 'source-in', 'source-out', 'destination-over', 'destination-atop', 'destination-in', 'destination-out', 'lighter', 'copy' or 'xor' http://www.w3schools.com/tags/canvas_globalcompositeoperation.asp

avandecreme · 2016-01-06T23:54:09Z

src/tiledimage.js

@@ -1301,7 +1317,8 @@ function drawTiles( tiledImage, lastDrawn ) {
        drawDebugInfo( tiledImage, lastDrawn );
        return;
    }
-    var useSketch = tiledImage.opacity < 1;
+    var useSketch = (tiledImage.compositeOperation == 'source-over') ? (tiledImage.opacity < 1):true;


This seems more readable to me:

var useSketch = tiledImage.opacity < 1 || tiledImage.compositeOperation !== 'source-over';

avandecreme · 2016-01-06T23:58:07Z

This looks good to me. Note that you need to do a manual merge because #764 modified the same part of the code.

iangilman · 2016-01-07T19:18:37Z

src/openseadragon.js

+  * @property {String} [compositeOperation='source-over']
+  *     Valid values are 'source-atop', 'source-in', 'source-out',
+  *     'destination-over', 'destination-atop', 'destination-in',
+  *     'destination-out', 'lighter', 'copy' or 'xor'.<br>


I don't think we need the <br>.

iangilman · 2016-01-07T19:25:33Z

This is a great start! Just a few things to sort out. :-)

For the record: this fixes #765.

iangilman · 2016-01-14T17:41:16Z

@meihuisu Sorry, didn't see you had updated the pull request! Your changes look good; you just need to merge in the latest from master and we should be good to go :)

meihuisu · 2016-01-14T21:06:56Z

done.

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Thursday, January 14, 2016 9:41:20 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@meihuisu Sorry, didn't see you had updated the pull request! Your changes look good; you just need to merge in the latest from master and we should be good to go :)

Reply to this email directly or view it on GitHub:
#814 (comment)

meihuisu · 2016-01-14T22:41:17Z

while testing with our code, we are seeing this message on console with latest master..

[OpenSeadragon.Spring] options.animationTime must be a non-zero number openseadragon.js:14938:1

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Thursday, January 14, 2016 9:41:20 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@meihuisu Sorry, didn't see you had updated the pull request! Your changes look good; you just need to merge in the latest from master and we should be good to go :)

Reply to this email directly or view it on GitHub:
#814 (comment)

avandecreme · 2016-01-14T23:34:45Z

Yep, see 28d49df#commitcomment-15429375

iangilman · 2016-01-15T17:16:00Z

src/drawer.js

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @lends OpenSeadragon.Drawer.prototype */{

        this.context.save();
        this.context.globalAlpha = opacity;
+        this.context.globalCompositeOperation = compositeOperation;


compositeOperation is optional, so we should only set it to the context if it exists.

Also, please add compositeOperation to the doc comment here.

Is it better to explicitly have compositionOperation to default to 'source-over' and
so the use of it does not need to be checked? Or else, the setting of useSketch would
also need to have a check before its use.

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Friday, January 15, 2016 9:16:07 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @Lends OpenSeadragon.Drawer.prototype */{

this.context.save(); this.context.globalAlpha = opacity;

this.context.globalCompositeOperation = compositeOperation;

compositeOperation is optional, so we should only set it to the context if it exists.

Also, please add compositeOperation to the doc comment here.

Reply to this email directly or view it on GitHub:
https://github.com/openseadragon/openseadragon/pull/814/files#r49878837

It is set to default in openseadragon.js

1073 // APPEARANCE
1074 opacity: 1,
1075 compositeOperation: 'source-over',
1076 placeholderFillStyle: null,

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Friday, January 15, 2016 9:16:07 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @Lends OpenSeadragon.Drawer.prototype */{

this.context.save(); this.context.globalAlpha = opacity;

this.context.globalCompositeOperation = compositeOperation;

compositeOperation is optional, so we should only set it to the context if it exists.

Also, please add compositeOperation to the doc comment here.

Reply to this email directly or view it on GitHub:
https://github.com/openseadragon/openseadragon/pull/814/files#r49878837

It needs to be optional to this function, because arguments in front of it are optional. Even though this function is currently only used in one place, and all argument are passed in that one place, we need to think of the possibility someone else might call it and not pass in all the arguments.

If it's not passed into this function, I would prefer not to set it at all, rather than setting it to a default value. Depending on the canvas implementation, setting it when we don't need to may have a performance impact. Nothing else in OSD changes the globalCompositeOperation, and this function wraps the change in a context.save context.restore pair, so the change won't ever bleed out past this call, so we can be confident that it'll always be already set to the default when this function starts.

Does that make sense?

Okay.
----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Monday, January 18, 2016 9:18:32 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @Lends OpenSeadragon.Drawer.prototype */{

this.context.save(); this.context.globalAlpha = opacity;

this.context.globalCompositeOperation = compositeOperation;

It needs to be optional to this function, because arguments in front of it are optional. Even though this function is currently only used in one place, and all argument are passed in that one place, we need to think of the possibility someone else might call it and not pass in all the arguments.

If it's not passed into this function, I would prefer not to set it at all, rather than setting it to a default value. Depending on the canvas implementation, setting it when we don't need to may have a performance impact. Nothing else in OSD changes the globalCompositeOperation, and this function wraps the change in a context.save context.restore pair, so the change won't ever bleed out past this call, so we can be confident that it'll always be already set to the default when this function starts.

Does that make sense?

Reply to this email directly or view it on GitHub:
https://github.com/openseadragon/openseadragon/pull/814/files#r50023838

I also changed the default option value to null in openseadragon.js.

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Monday, January 18, 2016 9:18:32 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @Lends OpenSeadragon.Drawer.prototype */{

this.context.save(); this.context.globalAlpha = opacity;

this.context.globalCompositeOperation = compositeOperation;

It needs to be optional to this function, because arguments in front of it are optional. Even though this function is currently only used in one place, and all argument are passed in that one place, we need to think of the possibility someone else might call it and not pass in all the arguments.

If it's not passed into this function, I would prefer not to set it at all, rather than setting it to a default value. Depending on the canvas implementation, setting it when we don't need to may have a performance impact. Nothing else in OSD changes the globalCompositeOperation, and this function wraps the change in a context.save context.restore pair, so the change won't ever bleed out past this call, so we can be confident that it'll always be already set to the default when this function starts.

Does that make sense?

Reply to this email directly or view it on GitHub:
https://github.com/openseadragon/openseadragon/pull/814/files#r50023838

@iangilman, I am sorry, I think I committed something bad. can you revert it? thanks.

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Monday, January 18, 2016 9:18:32 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

@@ -383,6 +383,7 @@ $.Drawer.prototype = /** @Lends OpenSeadragon.Drawer.prototype */{

this.context.save(); this.context.globalAlpha = opacity;

this.context.globalCompositeOperation = compositeOperation;

It needs to be optional to this function, because arguments in front of it are optional. Even though this function is currently only used in one place, and all argument are passed in that one place, we need to think of the possibility someone else might call it and not pass in all the arguments.

If it's not passed into this function, I would prefer not to set it at all, rather than setting it to a default value. Depending on the canvas implementation, setting it when we don't need to may have a performance impact. Nothing else in OSD changes the globalCompositeOperation, and this function wraps the change in a context.save context.restore pair, so the change won't ever bleed out past this call, so we can be confident that it'll always be already set to the default when this function starts.

Does that make sense?

Reply to this email directly or view it on GitHub:
https://github.com/openseadragon/openseadragon/pull/814/files#r50023838

iangilman · 2016-01-15T17:16:30Z

Sorry, just caught one more thing. Otherwise looking good.

iangilman · 2016-01-15T17:21:02Z

I've fixed that assert, btw:

#820

Thanks for mentioning it :)

…ver'

iangilman · 2016-01-20T17:15:49Z

src/drawer.js

@@ -383,6 +384,9 @@ $.Drawer.prototype = /** @lends OpenSeadragon.Drawer.prototype */{

        this.context.save();
        this.context.globalAlpha = opacity;
+        if (compositeOperation !== undefined && compositeOperation !== null) {


This should probably just check to see if it's truthy: if (compositeOperation)

iangilman · 2016-01-20T17:21:23Z

Great minds think alike! Looks like you fixed the thing I was commenting on. :)

I do think the test should be truthy/falsy rather than explicitly for null.

I don't see any reason why the tests are failing, so I restarted them to see if it was just random.

meihuisu · 2016-01-20T17:29:01Z

It was caused by taking out the explicity default setting for compositeOperation in openseadragon.js
to 'source-over' yesterday and everything spiraled down. My first time with outside opensource and
github. I will switch to truthy/falsy after test it out with my viewer.

----- Original Message -----
From: "Ian Gilman" notifications@github.com
To: "openseadragon/openseadragon" openseadragon@noreply.github.com
Cc: "meihuisu" mei@isi.edu
Sent: Wednesday, January 20, 2016 9:21:29 AM
Subject: Re: [openseadragon] adding setting of globalCompositeOperation for tiledImage (#814)

Great minds think alike! Looks like you fixed the thing I was commenting on. :)

I do think the test should be truthy/falsy rather than explicitly for null.

I don't see any reason why the tests are failing, so I restarted them to see if it was just random.

Reply to this email directly or view it on GitHub:
#814 (comment)

iangilman · 2016-01-20T17:31:13Z

@meihuisu That makes sense. It's coming together nicely!

meihuisu · 2016-01-20T22:31:03Z

I am done.

iangilman · 2016-01-21T17:07:27Z

Awesome...thank you for getting all the details sorted out; it's a lovely patch!

adding setting of globalCompositeOperation for tiledImage

meihuisu changed the title ~~adding setting of globalCompositeOperation for tiledImagesUpdate to allow setting of globalCompositeOperation when~~ adding setting of globalCompositeOperation for tiledImage Jan 6, 2016

avandecreme reviewed Jan 6, 2016
View reviewed changes

iangilman reviewed Jan 7, 2016
View reviewed changes

1) update according to comments from openseadragon's

087507c

Mei-Hui Su added 2 commits January 14, 2016 12:32

Merge https://github.com/openseadragon/openseadragon

3cfe8ec

fix couple of typos

e89d1f4

iangilman reviewed Jan 15, 2016
View reviewed changes

Mei-Hui Su added 6 commits January 15, 2016 10:58

Merge https://github.com/openseadragon/openseadragon

a3acaf4

Make setting of globalCompositeOperation optional in drawer.js

efc9098

change option compositeOperation default to null instead of 'source-o…

c214e2f

…ver'

put back the 'options.'

41ee1e2

Fix the optional check to include null

ac0438e

Add check for null compositeOperation option when setting useSketch

91ecd80

iangilman reviewed Jan 20, 2016
View reviewed changes

simplify the checks on compositeOperation

69cce57

iangilman added a commit that referenced this pull request Jan 21, 2016

Merge pull request #814 from informatics-isi-edu/master

0a61e16

adding setting of globalCompositeOperation for tiledImage

iangilman merged commit 0a61e16 into openseadragon:master Jan 21, 2016

iangilman added a commit that referenced this pull request Jan 21, 2016

Changelog for #814

8d453b5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding setting of globalCompositeOperation for tiledImage #814

adding setting of globalCompositeOperation for tiledImage #814

meihuisu commented Jan 6, 2016

avandecreme Jan 6, 2016

iangilman Jan 7, 2016

avandecreme commented Jan 6, 2016

iangilman Jan 7, 2016

iangilman commented Jan 7, 2016

iangilman commented Jan 14, 2016

meihuisu commented Jan 14, 2016

meihuisu commented Jan 14, 2016

avandecreme commented Jan 14, 2016

iangilman Jan 15, 2016

meihuisu Jan 15, 2016

meihuisu Jan 15, 2016

iangilman Jan 18, 2016

meihuisu Jan 19, 2016

meihuisu Jan 19, 2016

meihuisu Jan 19, 2016

iangilman commented Jan 15, 2016

iangilman commented Jan 15, 2016

iangilman Jan 20, 2016

iangilman commented Jan 20, 2016

meihuisu commented Jan 20, 2016

iangilman commented Jan 20, 2016

meihuisu commented Jan 20, 2016

iangilman commented Jan 21, 2016

adding setting of globalCompositeOperation for tiledImage #814

adding setting of globalCompositeOperation for tiledImage #814

Conversation

meihuisu commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avandecreme commented Jan 6, 2016

Choose a reason for hiding this comment

iangilman commented Jan 7, 2016

iangilman commented Jan 14, 2016

meihuisu commented Jan 14, 2016

meihuisu commented Jan 14, 2016

avandecreme commented Jan 14, 2016

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

iangilman commented Jan 15, 2016

iangilman commented Jan 15, 2016

Choose a reason for hiding this comment

iangilman commented Jan 20, 2016

meihuisu commented Jan 20, 2016

iangilman commented Jan 20, 2016

meihuisu commented Jan 20, 2016

iangilman commented Jan 21, 2016