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

Add opacity support. #644

Merged
merged 5 commits into from Apr 29, 2015
Merged

Add opacity support. #644

merged 5 commits into from Apr 29, 2015

Conversation

avandecreme
Copy link
Member

I just started from a fresh fork, so this is a new PR but this is the continuation of #630.

@avandecreme
Copy link
Member Author

@philipgiuliani How can I test that my modifications are not breaking anything with setClip and placeholderFillStyle?

Also, I believe the opacity property on the drawer is deprecated. This was useful when we had multiple drawers.

* Sketch canvas used to temporarily draw tiles which cannot be drawn directly
* to the main canvas due to opacity.
*/
this.sketchCanvas = this.useCanvas ? document.createElement( "canvas" ) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Since adding a new Canvas can have a big impact on our memory footprint, I think it would be good if we could do it only as needed, i.e. the first time we have a fractional opacity.

@iangilman
Copy link
Member

I love it.

Of course there are details to attend to. In addition to the comments above, a few more:

  • Please document opacity for the TiledImage constructor and for Viewer.addTiledImage.
  • TiledImage needs a setOpacity and getOpacity.
  • I agree that Drawer.opacity (and its getter/setter) should be deprecated. Generally while deprecating, I like to see if I can hack in the behavior (along with a deprecation message to the console) so we don't break existing code (but do warn that it will eventually break). For Drawer.setOpacity you could loop through the world and setOpacity on all the items. For Drawer.getOpacity you could loop through the world and return the max opacity.
  • We might as well still support the opacity global option for Viewer...this is the easiest way for people with single images. It would simply be the default opacity used in addTiledImage if it wasn't explicitly overridden for that function (much like placeholderFillStyle is done).
  • It looks like you're doing the right thing to support placeholderFillStyle and setClip. To test placeholderFillStyle, you could pass in an empty tileSource plus a placeholderFillStyle...that should make it so it's always drawing the placeholder since the tiles will never show up. For setClip, just pass in a clip rectangle with your tileSource (it's in image coordinates) and see if it clips properly.

@avandecreme
Copy link
Member Author

I think I covered all of your comments in the above commit.

I am not too happy with drawer.clear which seems inconsistent to me. Did you have that in mind?

I still need to test placeholderFillStyle and setClip, thanks for the pointers.
Ooops, and write/fix the tests :)

@iangilman
Copy link
Member

I love how this is coming together! I'm looking forward to using this feature. :)

@avandecreme
Copy link
Member Author

Ok, I think this is ready for merging.

@iangilman
Copy link
Member

Looks good! The Travis build glitched, so I've restarted it. I'll merge if the result is good. Thanks again for tackling this!

iangilman added a commit that referenced this pull request Apr 29, 2015
@iangilman iangilman merged commit 019d82a into openseadragon:master Apr 29, 2015
iangilman added a commit that referenced this pull request Apr 29, 2015
@avandecreme avandecreme deleted the opacity branch April 21, 2016 15:14
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.

None yet

2 participants