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

improved mirroring #484

Merged
merged 6 commits into from
Nov 19, 2024
Merged

improved mirroring #484

merged 6 commits into from
Nov 19, 2024

Conversation

barrettMCW
Copy link
Contributor

Fixed the jitter from panning while rotated, and it seems at some point in the pr process drawing the rois while mirrored broke. Fixed it up. Still couldn't get projections to work. I do think that would be the most robust way to handle this but I'll take another crack at projections next time something breaks. Thanks y'all!

@will-moore
Copy link
Member

Thanks for this - apologies for the delayed response.
It's working nicely and fixes the crazy jitter on dragging when rotated. It also fixes the ROI drawing when flipped.
However, the dragging of the image when flipped is opposite to the expected direction (the flipping is not taken into account to reverse the direction of drag).

NB: I also found it a little tricky to know when the image was flipped as there's no indicator. This is outside the scope of the PR so no need to address it now unless you want to, e.g. add some visual clue to the flip buttons when they are enabled?

@barrettMCW
Copy link
Contributor Author

That's strange... I added a mitigation specifically to fix that on my end line 80

Can you give me some more info about how you're testing it? I'm viewing it with npm run dev on chrome.
I like the flipped indicator idea! I'll add it quick.

@will-moore
Copy link
Member

That's weird. I'm building with npm run debug or npm run prod then opening iviewer from webclient.
However, if I use npm run dev then I don't see the bug (same as you).

@barrettMCW
Copy link
Contributor Author

added flip indicator, not pretty lol but distinct between flipped and not flipped.
the prod vs dev is a real head scratcher though, I'll keep looking into it

@barrettMCW
Copy link
Contributor Author

Think I might have gotten it figured out? I was looking at the name of the constructor which gets minified in prod. Hopefully that was it, I don't have an easy way of testing this out, let me know if it's still an issue then I'll set up a proper dev environment to test this more thoroughly.
Thanks!

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

That's fixed it - thanks!
Looking good - approved.

I noticed one tiny issue which I'll mention in case you can find a fix (but isn't a blocker on this PR which already has plenty of fixes)... The blue rectangle that appears when you hold Command-drag to select a bunch of ROIs is flipped on the screen (although the ROIs are correctly selected from the area that was actually dragged over).

@barrettMCW
Copy link
Contributor Author

Great catch! Thanks! (didn't know you could do that lol) It's mostly fixed. It captures extents properly but for whatever reason it doesn't render when only X is mirrored. When just Y or X and Y are mirrored it works perfectly but X is misbehaving. Still captures properly so I think it's better than it was.

@barrettMCW
Copy link
Contributor Author

Still haven't figured out roi popups yet. I'll skip that this PR, but if there's anything else you find, let me know. Thanks!

@Tom-TBT
Copy link
Contributor

Tom-TBT commented Oct 11, 2024

Hello,
I tried it out and it works quite well now, congrats.

A few things that I spotted.

When you click a button, it stays selected until something elsewhere is clicked. I find it slightly annoying. It would be nice if it could keep the "unselected" look right after clicking on it.
image

About the button color, I tried with different blues. I could suggest the "dodgerblue".
But if I'm being honest my visual tastes can be poor. Up to you to accept the suggestion or not.
image

To finish, I found an actual bug: the text of the label ROIs are flipped too 😅
image

Cheers,
Tom

@barrettMCW
Copy link
Contributor Author

@Tom-TBT Thanks a lot! I appreciate it! I like the color change, I'll add that. I was aware of the other two issues, the button thing is just weird and the text thing seems tricky, but I'll take a peak. The button issue can probably get solved, but the text issue seems like it will need the proper projection based flipping to keep the position accurate while displaying the text right side up, although I'll take a stab at it.

@barrettMCW
Copy link
Contributor Author

Sorry for the delay. Changed the color but I don't think I'll be able to fix the text issue or the selected issue. The text is ol native, so I'm not sure how to go about extending or modifying that and the selected issue seems to effect all the controls, such as zoom and birdseye, even on the IDR. I created #487 to track it.
But thanks for spotting those @Tom-TBT!

@barrettMCW
Copy link
Contributor Author

Just discovered that the scalebar can be dragged, and of course, it drags wrong when flipped, checking it out now.

@barrettMCW
Copy link
Contributor Author

Sorry, got busy and this fell off, won't be able to put time into this for a bit. I think this is a good place to call it for this pr.
Thanks for all the help y'all!

@will-moore
Copy link
Member

This contains a bunch of fixes so it would be good to get this in.
Although some minor issues were identified during the testing, they are not sufficient to block this and can be tackled in future when time allows.

@will-moore will-moore merged commit dfd7b91 into ome:master Nov 19, 2024
1 check passed
@will-moore will-moore added this to the 0.15.0 milestone Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants