Circle selector addition #17
Conversation
… and also the centre isn't properly selected
…he enclosing box and moving the region entirely
…onality is fine but thinking to change the way code is written
| Pushed some changes to this PR. Now, the functionality looks fine. @seveibar, Please review once and suggest if any changes are required. |
| looks solid at first glance, going to do deeper review tonight |
There was a problem hiding this comment.
overall, this PR is really good. There are some minor changes that I would take before merging, but I can take these steps prior to the next release. @saichander17 let me know if you'd like to continue work on this PR with the comments I've posted so far or pass this PR to be completed by me prior to the next release. Thanks for the great work!
Also sorry for the delay, it was a U.S. holiday this past week :) 🦃
| y: number, | ||
| // xr and yr indicate the x and y coordinates of the current mouse pointer while dragging. | ||
| xr: number, | ||
| yr: number |
There was a problem hiding this comment.
I'm lukewarm on this definition because technically in a circle xr=yr so it should really just have r. My suggestion is to have an optional r?: number, which is set in the case where xr=yr to be r=xr=yr because this is more consistent with expectation when dealing with circles.
There was a problem hiding this comment.
@seveibar, I started with a single r as you are suggesting but, since we are transforming the coordinates based on image height and width, single r won't be enough.
In the current code, xr and yr aren't actually representing the radius exactly but are representing the coordinates to last point while dragging by a mouse.
| export type Region = Point | PixelRegion | Box | Polygon | ||
| | ||
| export const getEnclosingBox = (region: Region) => { | ||
| export const getEnclosingBox = (region: Region, iw: number, ih: number) => { |
There was a problem hiding this comment.
it's unclear to me why iw and ih are needed (because they're not needed for other structures). It seems like the enclosing box should be equal to...
{ x: region.x - region.xr, y: region.y - region.yr, w: region.xr, h: region.yr } If possible, we should remove iw and ih from the parameters here.
There was a problem hiding this comment.
As I mentioned in my previous comment, xr and yr aren't actually representing the radius exactly but are representing the coordinates to last point while dragging by a mouse. So, I need ih and iw.
{ x: region.x - region.xr, y: region.y - region.yr, w: region.xr, h: region.yr }
This works if xr is x-radius and yr is y-radius.
I'm thinking to make xr as x-radius and yr as y-radius but then, I'd need ih and iw in the reducer. I'm new to react and redux. So, I'd need your advice here as to how I can set ih and iw in the state (store) of the reducer.
Reason I need ih and iw in the reducer:
Based on the center of the circle and the current point, I have to calculate x-radius and y-radius.
…instead_of_point ghost code removed
…instead_of_point circle region selector icon changed
| I've started looking at this code more closely and interacting with the code myself. The big issue here is that the radius cannot be represented in the "percentage" unit or the resulting circle is not a circle, but rather an oval squashed at a ratio equal to the width:height ratio of the image. |
| so i'm going to close this and open a new PR from our project because i discovered new issues and made some progress towards our radius issue. @saichander17 you're welcome to push to the new PR (I given you permissions), there are only mostly minor issues remaining. |
| Thanks @seveibar. |
2 participants
In this, there is an issue with the enclosing box. It's usually much bigger in size. Should only be merged once it's fixed