DM-10065: Client side Rotation and Flipping #348
Conversation
|
Files to review by reviewer: @loitly :
Less Important / Small changes / code cleanup:
Removed unused files:
|
|
I've built and test randomly the tri-view. So far i've found the following:
|
|
Another issue is that i've noticed that the footprint rotation is broken. |
That was noted in known issues at the top of the pull request. There is a list 3 issues we will do in separate tickets. |
| pt0 = plot.getViewPortCoords(wpt0); | ||
| pt = plot.getViewPortCoords(wpt); | ||
| pt0 = plot ? plot.getDeviceCoords(wpt0) : wpt0; | ||
| pt = plot ? plot.getDeviceCoords(wpt) : wpt; | ||
| if (!pt0 || !pt) return; |
There was a problem hiding this comment.
should here return with 'if (!onlyAddToPath) DrawUtil.stroke(ctx);'?
|
Found a bug: After rotation, the restore key can't rotate the image back to the default. How to repeat: |
Good catch. I did not even think of restore. Now that I think about it that is a little tricky. In the Time Series case I don't think we want to unrotate. |
|
Found another bug: Now the rotation is done by the given angle relative to the original position. For example, rotate 45deg, then rotate 90deg, the result is only rotating 90 deg. Or rotate 45 deg, then another 45 deg, the image has no change for the second rotation. Before the rotation is relative to the current position. |
|
It seems that the rotate angle entered into the 'Rotate Image' popup window means the angle relative to the original image. Will that be good to have some number shown on the popup window indicating the current rotate angle, then the user could enter the rotation angle in addition to current rotate angle? |
It is already showing on the title. We could do it in the dialog as well but I think we might want the to be another ticket. |
There was a problem hiding this comment.
It works really well. I did notice a couple of things. It's perfectly fine to merge if they are recorded in another ticket.
Things I noticed from testing:
-
on Safari, if you zoom an image so that it tiles on the server, you will notice 1-2px gap(s) along the borders.
-
bring up 2 same images (IRAS m31);
click on 'WCS Match';
move one off screen;
click on 'Recenter'; only one returns;
clicked on the other one, image returned, but warning showed up on the other.
this worked fine in 'dev' branch.
| onImageLoad(e) { | ||
| if (e) { | ||
| e.onload= () => { | ||
| this.setState( () => ({imWidth: e.width, imHeight:e.height })); |
There was a problem hiding this comment.
Is this right? You are sending a function to setState. Shouldn't it be the returned object?
There was a problem hiding this comment.
Loi and I just talked about this. This is a new way to setState in react.
The new way is more consistent with DS9. I think it is the right way to go. |
|
Indeed. From
https://facebook.github.io/react/docs/react-component.html#setstate
*The first argument can be an object (containing zero or more keys to
update) or a function (of state and props) that returns an object
containing keys to update.*
Though in your case, is there an advantage of using a function rather than
an object?
2017-04-11 16:11 GMT-07:00 Trey Roby <[email protected]>:
… ***@***.**** commented on this pull request.
------------------------------
In src/firefly/js/visualize/ui/ThumbnailView.jsx
<#348 (comment)>:
> @@ -39,6 +41,14 @@ export class ThumbnailView extends Component {
return this.drawData;
}
+ onImageLoad(e) {
+ if (e) {
+ e.onload= () => {
+ this.setState( () => ({imWidth: e.width, imHeight:e.height }));
Loi and I just talked about this. This is a new way to setState in react.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#348 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALU3QGOm84HP72IiIKBhXdVKc1Oq3CSuks5rvAiLgaJpZM4M5SiK>
.
|
|
No advantage for what I was doing it. However, for the next major release of react they are encouraging this to be the main way of setting state. There is some performance optimization this helps enable. |
|
review completes.
|
Yes, I will look into it. |
|
More issues:
*Need further discussion about the rotation be relative to the original or additional to the current position. In other words, we should follow DS9 way or IRSA way (see IRSA help above). Continue to test ... |
|
Also the rotation angle should be between 0 and 360 degrees. Negative angle is not allowed. Input a negative angle, one should see the error message. |
|
Yi wrote:
Footprints are being handle in another ticket. see: https://jira.lsstcorp.org/browse/DM-10188
Will fix this one. |
|
The post-rotation cropping frames are confusing: one dashed and one solid. It seems the cropping is done by the dashed one. Do we need the solid frame? See attached image. I guess the solid frame is for showing the user's mouse dragging?
|
I am open to suggestions. We need to crop on the original image. Therefore when you make a selection now there are some actions that can happen on the selected area like selecting points but crop cropping need to be in original image bounds. |
|
Post rotation flipping issue: After rotation, the flip is done on the north-south direction (if the image is north up originally ) or the original up-down direction. A bug? In IRSA, the flip is alway on the Y axis (up and down direction) before or after rotation. |
This way is more consistent with DS9. I think firefly was doing it wrong before. I am not sure what others do but I think users will expect it to be more like how DS9 does it. |
|
Before when multiple images are locked as a group, rotation is still done for the selected image only. This version the rotation is done for the whole group which locked. New feature of bug? |
|
After loading some images, right click and select "Reload", this is for reloading the images (IRSA wise way) or going back to search (the current version)? Even it's for going back to search, the search panel's "Cancel" key is not working. BTW. The catalog "Reload" key on the coverage map reloads the try-view, instead of going back to search. But after loading some images, this coverage map reloading jumps to search. |
The way we did it before has been criticized. We only did it that way because it was so CPU on the server. Now it seems to make sense to keep the group lock like everything else. Yi comments on Trey's above: |
| @@ -99,12 +95,14 @@ const ROTATE_START= `${PLOTS_PREFIX}.RotateChangeStart`; | |||
| const ROTATE= `${PLOTS_PREFIX}.RotateChange`; | |||
| const ROTATE_FAIL= `${PLOTS_PREFIX}.RotateChangeFail`; | |||
There was a problem hiding this comment.
I see action creators and reducer behavior for these actions commented, but not actions themselves.
Are we going to use ROTATE_CLIENT and FLIP_CLIENT behavior for ROTATE and FLIP later?
There was a problem hiding this comment.
I will remove the old constants. Maybe I should make the client ones the old ones. I will think about it.
There was a problem hiding this comment.
I think I will make the just to be ROTATE and FLIP
| //------------------- | ||
|
|
||
|
|
||
| //------------------- |
There was a problem hiding this comment.
Is there a TODO missing between comment lines?
| switch (mouseState) { | ||
| case DOWN : | ||
| dispatchChangeActivePlotView(plotId); | ||
| const {scrollX, scrollY}= this.props.plotView; |
There was a problem hiding this comment.
this.props.plotView can be plotView, since you are passing it as an argument now
| var {width:viewPortWidth,height:viewPortHeight}= plot.viewPort.dim; | ||
| const {left,top,scrollViewWidth, scrollViewHeight, scrollX,scrollY, viewDim}= getPositionInfo(plotView); | ||
|
|
||
| const {plotId, viewDim:{width,height}}= plotView; |
There was a problem hiding this comment.
nice shortcut for width and height initialization
| if (!found) { | ||
| found= screenAsDevAry.some( (pt) => contains(0,0,viewDim.width, viewDim.height,pt.x,pt.y) || | ||
| intersects(0,0,viewDim.width, viewDim.height,pt.x,pt.y,1,1)); | ||
| } |
There was a problem hiding this comment.
In functions like this, I wish there would be some human readable explanation of code logic.
There was a problem hiding this comment.
I will write a description. I had not because it took me awhile to convince myself the concept worked.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
I would call this file PlotTransformUtils.js - it would better describe what it's for.
There was a problem hiding this comment.
good idea. This file was a work-in-progress for a long time. I will give it a better name.
| * @return {Number} | ||
| */ | ||
| getRotationAngle() { return this.rotationAngle; } | ||
| // getRotationAngle() { return this.rotationAngle; } |
There was a problem hiding this comment.
How should can we get the rotation angle now? (I know we rely on PlotState a lot in API.)
There was a problem hiding this comment.
plotView.rotation and plotView.flipY
src/firefly/js/visualize/Point.js
Outdated
| @@ -22,7 +22,17 @@ var Point = { SPT, IM_PT, IM_WS_PT, VP_PT, PROJ_PT, W_PT, OFFSET_PT}; | |||
| * | |||
| * @prop {Number} x | |||
| * @prop {Number} y | |||
| * @prop {String} type one of 'ScreenPt', 'ImagePt', 'ImageWorkSpacePt', 'WorldPt', 'ViewPortPt', 'ProjectionPt', 'OffsetPt' | |||
| * @prop {String} type one of 'RenderedPt', 'ScreenPt', 'ImagePt', 'ImageWorkSpacePt', 'WorldPt', 'ViewPortPt', 'ProjectionPt', 'OffsetPt' | |||
There was a problem hiding this comment.
Is ViewPortPt completely gone? It is still referenced here.
There was a problem hiding this comment.
yes, I will clean up the docs
src/firefly/js/visualize/VisUtil.js
Outdated
| return testPts.every( (p) => pointInPolygon([p.x,p.y], polyPtsAsArray)); | ||
| } | ||
|
|
||
| export function findIntersectionPt(x1, y1, x2, y2, x3, y3, x4, y4) { |
There was a problem hiding this comment.
We need jsdoc or comments here to tell which pairs of (x,y) make first and second segments
| }); | ||
| const plot= primePlot(plotView); | ||
| const {x,y}= iPt; | ||
| if (x >=0 && y >= 0 && x < plot.dataWidth && y < plot.dataHeight) { |
There was a problem hiding this comment.
What is this check for? To make sure it's a valid image point?
There was a problem hiding this comment.
is is support to do the same as line 140. I meant to fix both. It check to see it the image point is on the plot. It should call pointInPlot
|
I found sometimes the rotation button has no response when I click it. This happened after rotation, zoom, crop, was/target match, etc. I can't repeat the problem. See the error messages I got in the attached file. |
|
I see the place in the code but I am not sure how it could get there. If you could figure our how to repeat is would help. I wonder if you have a series of exceptions and it is really the first exception that is creating this problem. |
|
Sorry I tried but I really can't repeat the problem. I think I am done with this version's testing. Found some minor issues but I rather to double check them in next version. They are hard to repeat also. |
- the html5 canvas will now rotate the image and flip - package.json: added transformation-matrix-js - package.json: added point-in-polygon - more work in understanding how to rotate - moving viewport render calculation to TileDrawer, scroll working now - thumbnail working - new DevicePt coordinate system - removed ViewportPt coordinate system - drawing layers mostly working - select and selection related operations working with rotation - select now has a second box that shows when image is rotated, this is used for cropping and image stats - rotate dialog, rotate north using new rotation - wcs match working - target match working - time series working - compass working
- fixed rotate north not working in certain cases - fixed restore not unflipping and unrotating - fixed recentering problem when WCS match is on - fixed magnifier: now rotated and flipped - fixed round off error that made gaps in the image tiles of safari and firefox - fixed rotation issues with wcs match when already rotated - fixed rotation match checking false positives - png download now works - flip icon now is on and off - rotation angle/direction now consistent with ds9 and irsa standard - scrolling speed optimization using lodash throttle - added more documentation on a couple of functions - rename PlotPostionUtil to PlotTransformUtils - actions are now be ROTATE and FLIP - code clean up
|
Completed the test on this new version. This version fixed a lot. My test report: |
…w rotation scheme
…e line with center information in footprint drawlayer UI. DM-10188 Fix vertextDef setting under the drawing layer for mutiple plots
DM-10188 Fix drawing related issues on markers and footprints with new rotation scheme
Dm-10241 Fix the rendering of east arrow in the North/East compass
Branch covers the following:
Testing:
Here are some functions that need special attention.
Known Issues that will be handle in separate ticket: