Firefly-1887: Support SPHEREx phtometry cutouts #1870
Conversation
|
(you were SO not kidding about the performance.. yikes. and it's not getting faster even if i'm accessing the same images, like changing the cutout size.) all the images come up for me ridiculously tiny. see screenshot. they should be at the 'zoom to fit' scale. when i change the cutout size, it changes which image is centered. this doesn't seem optimal. it should retain the same image highlighting and ordering as it had before i changed the cutout size. how is it deciding on orientation? because it didn't put north-up, but it also didn't align with the tile orientation. |
|
@lrebull I have seen the small size if you click on the browser while it is loading the first time. It is having trouble computing the size in the case. I don’t think that will be a long term problem. when you change the cutout size it recenters on the spectrum point. That seems like the right thing. |
I haven't been able to look at it (no VPN capability with me) but I feel strongly that users will expect that all the cutouts be oriented the same way, so that as you move from one spectrometry point to the next the features on the sky are in the same places. North-up would be the easiest to implement, I think? |
|
they are all oriented the same way, but not north up. evidently. at least the first time i loaded it. |
|
Maybe it's something like the "first" or "last" image in the spectrum is displayed in its "natural" row-column orientation, and then everything else is locked to it? |
|
They are suppose to be north up. Maybe the north up is getting unlock some way. I am seeing them north up. |
|
it may be another instance of "the first time i used it, strange things happen", like the zoom not being right. because now the zoom is right every time i load it. |
There was a problem hiding this comment.
This takes time to load as you mentioned but it works great! I tested different image counts and connections between image slider, spectrum table, and spectrum chart - they look good.
However, I noticed a UX issue with your changes to ImageTableRowViewer component. Steps to reproduce:
- Do a spectrophotometry search, wait for image cutouts to load.
- Click on the right slider arrow to slide the highlighted image/chart-point, it will come to the mid. Now clicking on right arrow further, will load a new image on right with image sliding animation. All good!
- Now from the spectrum chart, click on a point that's farther away from the current images view, somewhat close to the right end of spectrum. Instead of seeing the image sliding animation and then 5 skeleton boxes, you see a single skeleton box with "searching images" which blocks the slider arrows underneath too.
I find it awkward - the loading UI should be consistent in all different clicks in chart or slider. We want to establish it to user that this is an image slider UI at all times.
| if (hRowChangedByUI.current) { | ||
| const midSlideIdx = table?.highlightedRow; | ||
| layoutImages(viewerId, Number(imageCnt), table, makeRequestFromRow, midSlideIdx, cutoutWpt); | ||
| changeActivePlot(viewerId, table, midSlideIdx, activePlotChangedByUI); //synchronise active plot as per highlighted row |
There was a problem hiding this comment.
I don't see why you removed this?
I remember there were some cases in which changing the highlighted row from table view (not highlighting a new point from chart view) of spectrum, wasn't synchronizing the chart.
| else { //table.highlightedRow === midSlideIdx; move highlighted row/plot as well as slider | ||
| changeHighlightedRow(table, rowToHighlight, hRowChangedByUI); | ||
| layoutImages(viewerId, Number(imageCnt), table, makeRequestFromRow, rowToHighlight, cutoutWpt); | ||
| changeActivePlot(viewerId, table, rowToHighlight, activePlotChangedByUI); |
There was a problem hiding this comment.
I removed it because it is is now done in ImageTableRow view after the onPlotComplete promse.
There was a problem hiding this comment.
You mean L438-439?
I suspected that they were doing this but wanted to confirm.
| const finishedPromiseAry= exclusiveNewPlotIds.map( (plotId) => onPlotComplete(plotId)); | ||
| await Promise.all(finishedPromiseAry); |
There was a problem hiding this comment.
Why is it necessary to wait for all images in the slider view here and not let each image viewer handle it's loading state individually?
There was a problem hiding this comment.
There are race conditions that are not cleanly solvable. Sometime the active plot would be set to one associated with row zero. It is much, much cleaner to wait until the all load before setting the active plot or recentering.
| const WorkingAnimation= ({message}) => ( | ||
| <Box sx={{ position:'absolute', left:0, right:0, top:34, bottom:20}}> | ||
| <Skeleton variant='rectangular' sx={{width:1, height:1}}/> | ||
| <Typography {...{level:'h4', width:1, position:'absolute', top:'3rem', textAlign:'center', color:'neutral'}}> | ||
| {message} | ||
| </Typography> | ||
| </Box> | ||
| ); |
There was a problem hiding this comment.
This is cool addition - esp the use of Suspense.
But I'm not convinced that this is necessary here (also see my previous comment). The image slider was already showing Skeleton box for each image making it clear that this image is being loaded (since toolbar already says "Image count").
If you still want to keep it, I'd advise not to do this single rectangle skeleton that spans across entire width of slider hiding the slider arrows! You can instead use GridMask (a Skeleton wrapper Loi added a while ago) to show imageCount number of skeleton boxes, and place it so that the arrows on the left/right edges of the image slider visible.
There was a problem hiding this comment.
It is very much needed with the data link taking so long. I think it will be necessary even if they get the response faster. Otherwise the whole area appears to hang. That all happens before the dispatchPlotImage which is when you see the image working feedback. This is during the makeRequestFromRow call. Notice.makeRequestFromRow now returns a promise to a request since it involves a server call.
I will try GridMask. However, I don't think there is a good way a the above level to mask part of the area.
There was a problem hiding this comment.
Update: I think I can us the same approach inside the ImageSlider component. So I think this will address you concern.
| const plotId= newPlotIdAry.find( (id) => id.endsWith(midSlideIdx+'')); | ||
| if (plotId) dispatchChangeActivePlotView(plotId); |
There was a problem hiding this comment.
Since it's a replacement of changePlotView at 2 places above, shouldn't we call the changePlotView here which wraps dispatchChangeActivePlotView and handles some edge cases. IIRC activePlotChangedByUI ref must be set correctly to handle random order clicks, otherwise image slider and spectrum interaction suffers in some corner cases.
There was a problem hiding this comment.
Keeping the ref might no longer be necessary. since the race conditions are fix by using the promise array. I will test trying to take it out altogether.
- Enhance ImageTableRowViewer, works with request promises, improved working, improved updates - Fixed bug in managing worker thread. Sometimes it is deleted before finishing work resulting in render failure - includes response to feedback
robyww
force-pushed
the
FIREFLY-1887-cutouts
branch
from
9a5e990 to
4f5c0d7
Compare
4 participants
Firefly-1887: Support SPHEREx photometry cutouts
IFE PR: https://github.com/IPAC-SW/irsa-ife/pull/446
Testing