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 TiledImage.fitInBounds method. #888

Merged
merged 4 commits into from Mar 24, 2016

Conversation

avandecreme
Copy link
Member

I added a new "enum" Placement which is very similar to OverlayPlacement but with the added functionality to have handy boolean values.
I am thinking of replacing https://github.com/openseadragon/openseadragon/blob/master/src/overlay.js#L54-L64 by $.OverlayPlacement = $.Placement. It should be backward compatible provided people did not use the previously assigned numbers.

It would be nice to be able to set the bounds directly from the constructor (and consequently from viewer.addTiledImage) but it doesn't look easily doable. Suggestions welcome :)

@iangilman
Copy link
Member

Looks lovely! I'm down with updating OverlayPlacement as you suggest. I suppose the only risk is if people have serialized the values (or plan to in the future, since the new values don't lend themselves to that)... not sure if that's a likely scenario.

As for adding it to addTiledImage, I suppose we can have new image spec properties: fitBounds and fitBoundsPlacement and pass those through (making sure along the way that people don't pass both x and fitBounds, for instance). Of course the image spec options are already plentiful, so I'm hesitant to add more. On the other hand, it does seem like a common scenario.

@avandecreme
Copy link
Member Author

Looks lovely! I'm down with updating OverlayPlacement as you suggest. I suppose the only risk is if people have serialized the values (or plan to in the future, since the new values don't lend themselves to that)... not sure if that's a likely scenario.

Good point. I found that post http://stijndewitt.com/2014/01/26/enums-in-javascript/ to have the best of both worlds. And it will be 100% compatible with the current implementation.
We could also use Object.freeze when available (IE>8) for extra safety.

As for adding it to addTiledImage, I suppose we can have new image spec properties: fitBounds and fitBoundsPlacement and pass those through (making sure along the way that people don't pass both x and fitBounds, for instance). Of course the image spec options are already plentiful, so I'm hesitant to add more. On the other hand, it does seem like a common scenario.

I am willing to add it. Right now, I am adding the images at an approximate location before fixing it in the add-item event. Not very elegant.

@iangilman
Copy link
Member

That sounds like a great plan for the enums!

... and sure, let's add the new properties to addTiledImage.

@dwrogers
Copy link

I suppose the only risk is if people have serialized the values (or plan to in the future, since the new values don't lend themselves to that)... not sure if that's a likely scenario.

Serializing overlay placement is something that I am planning on doing and seems like it would be a common requirement. I don’t know about current uses. Couldn’t you just assign the indicated values to the placement objects, as in:

18, 36, 24, 12, 10, 9, 11, 21, 22 for CENTER through LEFT and then add functions to the placement object, as in isLeft(placement), isHorizontallyCentered(placement), isRight(... These functions could use a simple bitwise and to return the desired binary result.

That way you can easily serialze the overlay placement value and also get your binary values. You could also put a quick check for the old values before the “bitwise and” so that those functions could also handle the old values 0-9.

@dwrogers
Copy link

Sorry: “old values 0-9.” s/b “old values 0-8.”

David W. Rogers
Software Developer / Collaborative Data Services / 206.667.7089 / drogers@fredhutch.org / Fred Hutch / Cures Start Here

From: Rogers, David W
Sent: Tuesday, March 22, 2016 10:59 AM
To: 'openseadragon/openseadragon'
Subject: RE: [openseadragon] Add TiledImage.fitInBounds method. (#888)

Ø I suppose the only risk is if people have serialized the values (or plan to in the future, since the new values don't lend themselves to that)... not sure if that's a likely scenario.

Serializing overlay placement is something that I am planning on doing and seems like it would be a common requirement. I don’t know about current uses. Couldn’t you just assign the indicated values to the placement objects, as in:

18, 36, 24, 12, 10, 9, 11, 21, 22 for CENTER through LEFT and then add functions to the placement object, as in isLeft(placement), isHorizontallyCentered(placement), isRight(... These functions could use a simple bitwise and to return the desired binary result.

That way you can easily serialze the overlay placement value and also get your binary values. You could also put a quick check for the old values before the “bitwise and” so that those functions could also handle the old values 0-9.

David

David W. Rogers
Software Developer / Collaborative Data Services / 206.667.7089 / drogers@fredhutch.org / Fred Hutch / Cures Start Here

From: Ian Gilman [mailto:notifications@github.com]
Sent: Tuesday, March 22, 2016 10:04 AM
To: openseadragon/openseadragon
Subject: Re: [openseadragon] Add TiledImage.fitInBounds method. (#888)

Looks lovely! I'm down with updating OverlayPlacement as you suggest. I suppose the only risk is if people have serialized the values (or plan to in the future, since the new values don't lend themselves to that)... not sure if that's a likely scenario.

As for adding it to addTiledImage, I suppose we can have new image spec properties: fitBounds and fitBoundsPlacement and pass those through (making sure along the way that people don't pass both x and fitBounds, for instance). Of course the image spec options are already plentiful, so I'm hesitant to add more. On the other hand, it does seem like a common scenario.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHubhttps://github.com//pull/888#issuecomment-199909191

@avandecreme
Copy link
Member Author

That is an option as well. However, I am pretty bad with bitwise operations. How do you go about choosing the values and the operators?

Note that it will break again any serialization people currently may have been doing but I am planning on doing so anyway because OverlayPlacement.CENTER is currently 0. This is dangerous because in the following example, if placement is set to CENTER, it will be overwritten by TOP_LEFT.

function myFunc(placement) {
    placement = placement || OverlayPlacement.TOP_LEFT;
}

@dwrogers
Copy link

My bitwise operators are good - my javascript is newbie... :-)

The bitwise operations would be done against a "private" set of constants:

  {...
    placementMask: {
      isLeft: 0x20,
      isHorizontalCenter: 0x10,
      isRight: 0x08,
      isTop: 0x04,
      isVerticalCenter: 0x02,
      isBottom: 0x01
  }
(etc)
  isLeft: function(placement) { return placement & placementMask.isLeft; }
  isHorizontalCenter: function(placement) { return placement & placementMask.isHorizontalCenter; }
  isRight: function(placement) { return placement & placementMask.isright; }
 (etc).
}

@dwrogers
Copy link

The constants would be more like they are now - just with different values:

    $.OverlayPlacement = { 
      CENTER:       0x12, 
      TOP_LEFT:     0x24, 
      TOP:          0x14, 
      TOP_RIGHT:    0x0c, 
      RIGHT:        0x0a, 
      BOTTOM_RIGHT: 0x09, 
      BOTTOM:       0x11, 
      BOTTOM_LEFT:  0x21, 
      LEFT:         0x22 
      }; 

@avandecreme
Copy link
Member Author

I added the clipping handling. It should be ready to merge.

@iangilman
Copy link
Member

@avandecreme Awesome, looks great!

@iangilman iangilman merged commit 430804e into openseadragon:master Mar 24, 2016
iangilman added a commit that referenced this pull request Mar 24, 2016
@avandecreme avandecreme deleted the fit-in-bounds branch April 21, 2016 15:15
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

3 participants