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

Rendering using css transform instead of canvas transform #37

Closed
wants to merge 13 commits into from
Closed

Rendering using css transform instead of canvas transform #37

wants to merge 13 commits into from

Conversation

ghetolay
Copy link

Hi.
To follow this discussion, I tried to do some modification into cornerstone to avoid some issue I was having (especially with cached rendercanvas ).
So cornerstone rendering works as follow :
- Fill a "render" canvas with same size as the image with image pixel (applying lut etc..)
- Draw this "render" canvas into the final canvas and set all transformation the image on the final canvas
What I did was :
- Fill the "render" canvas like before except this canvas is also the final canvas displayed on screen.
- Set all transformation using css.

Pro:

  • No more cached render canvas.
  • Was able to improving rendering process reduces to just context.createImageData() and ``cornerstone.storedPixelDataToCanvasImageData()` (but because a Blink bug we may have to revert it back).
    • Transform (including matrix computing) done completely by the browser.
    • We never need to redraw image when we change transform properties.
    • Can use css transition to make fancy animation (e.g. flip).
    • Mouse events returns coordinate relative to image, no computation needed.
    • We can easily add DOM element relative to the image.

Con:

  • More memory needed in some case (when displaying several images bigger than the element they're displayed into)
  • Need more css rules
  • Does not work with IE 8
  • I've been trying to do annotation differently (drawing them above the image and not as part of the image rendering). This may require pointer-events available only from IE 11.
  • Had to add a wrapper
    around (this is embarrassing cause I know it's important but can't remember why I've done it... ).

This is not a real pull request, just a place to discuss about it then we will work on it if you're interested.
I've been working with this version of cornerstone on my viewer (on development/test) and it works really great for me.
You'll see some work about annotation, this part is really on an early stage. Even if I think cornerstone should offer an API to work with annotation, I just did it for the scrollzoompanwl example. So let's not talk about it, we'll see that later on.
Also it's the only example I've been working on so others may be broken.

Here is a link to the scrollzoompanwl example.

@ghetolay ghetolay changed the title Rendering using css transform instead of canvas Rendering using css transform instead of canvas transform Feb 11, 2016
@mko
Copy link

mko commented Feb 18, 2016

I've just recently started working with Cornerstone, and I like the move to CSS-based transformations, which is something I had been investigating when I came across this PR.

@chafey
Copy link
Collaborator

chafey commented Feb 23, 2016

This is on my list to review - stay tuned (big PRs take a while for me to review)

@ghetolay
Copy link
Author

ghetolay commented May 7, 2016

I've updated my PR by removing the container div called renderer and stuff related to my attempt to draw annotation.
I created a container div so we could add elements on the same space as the image. So if you add an element to this container, you could position it relatively to the image coordinate system. Then any transformation applied to the image (so to the container) will also automatically apply to the element.
But adding a div and starting to initiate how I will implements annotation was wrong. So I removed this container div and instead I'm keeping a second element (sibling to the canvas) in sync with the canvas transform through the CornerstoneTransformUpdated event.
For that matter I'm thinking about adding the transform string into the event datas.

Also I've corrected the transform computation done on calculateTransform() but had no time to really test it.

@ghetolay
Copy link
Author

ghetolay commented May 8, 2016

fitToWindow() Problem

I was struggling with the fitToWindow behavior because currently fitToWindow will fit and center the image displayed into the canvas, we can easily get both objects (canvas & image) size in order to do that.

Now on cssTransform fitToWindow means fit and center the canvas (which has the same size as the image) into the element. Problem comes when I tried to get the element size in order to compute the scale. Element is created and managed by the final user and there is way too much ways of getting the good width and height (using offsetWidth, offsetHeight, computedStyle, getBoundingClientRect()...) depending on how web design is done (do we want to include padding, border etc...).

So I think user must provide element's width and height for this to work. I also think we should not provide a default behavior like using offsetWidth/offsetHeight but force the user to compute itself the element width and height so he is totally aware of how this works and he knows it must adapt it if he change the web design. Avoiding issue tickets 'fitToWindow() won't work anymore' :)

Solution

Using viewport override on displayImage() and the now public scaleToFit() function, user can easily still have a fitToWindow behavior on image display like this :

cornerstone.displayImage(element, image, {
    scale: cornerstone.scaleToFit(element.offsetWidth, element.offsetHeight, image)
});

And calling fitToWindow will be done like this :

cornerstone.fitToWindow(element, element.offsetWidth, element.offsetHeight);

reset() problem

Problem is this create a problem for the reset(), since default scale is now 1, reset() will not apply the fitToWindow behavior. To resolve that we could ask the user to successively call reset() and fitToWindow() but didn't liked that.

Solution

Instead reset() will now reset the viewport into the last viewport defined by displayImage() including the override. I think it's good because it allows the user to define the 'initial viewport' and let him reset to that 'initial viewport' instead of reseting into our defined default viewport. Here is and illustrated example :

//We define the initial viewport (combination of defaultViewport() and override passed as argument)
cornerstone.displayImage(element, image, {
    scale: 5,
    invert: true,
    voi : {
        windowWidth: 2048,
        windowCenter: 1024,
   }
});

//Initial viewport is then 
//{
//    scale : 5,
//    translation : {
//        x : 0,
//        y : 0
//    },
//    voi : {
//        windowWidth: 2048,
//        windowCenter: 1024,
//    },
//    invert: true,
//    pixelReplication: false,
//    rotation: 0,
//    hflip: false,
//    vflip: false,
//    modalityLUT: image.modalityLUT,
//    voiLUT: image.voiLUT
//}

//We can modify the viewport as much as we want
....
cornerstone.setViewport( element, modified_viewport );

//will reset to the initial viewport shown before
cornerstone.reset(element);

As always I've just modified the scrollzoompanwl example.

@dannyrb
Copy link
Member

dannyrb commented Mar 24, 2019

This is a pretty old PR. While I think it's a neat spike, I'm not sure how practical it is. I would be open to looking at a revamped version of this if a good enough case could be made for the performance benefits.

@dannyrb dannyrb closed this Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 Change: Feature Change that expands the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants