Add aspect ratio locking and flip support to skin editor #12780
Conversation
| if (reference.HasFlagFast(Anchor.x0) || reference.HasFlagFast(Anchor.x2)) | ||
| scale.Y = scale.X; | ||
| else | ||
| scale.X = scale.Y; |
There was a problem hiding this comment.
What was the intention here? Asking because the else branch seems effectively dead here. I've breakpointed on it and it seems to be unreachable. To explain what I mean:
x0 x1 x2
| | |
y0 -O---O---O-
| | |
y1 -O---+---O-
| | |
y2 -O---O---O-
| | |
The Os indicate where the reference anchors are on a selection box. The first conditional eliminates the middle ones, which makes sense. But after excluding them from further deliberations (marking via X):
x0 x1 x2
| | |
y0 -O---X---O-
| | |
y1 -X---+---X-
| | |
y2 -O---X---O-
| | |
all the reference anchors left have Anchor.x0 or Anchor.x2...
There was a problem hiding this comment.
i'm not actually sure, but it felt okay. this code will likely be removed/replaced within the next week, so just killing that line is fine for now.
There was a problem hiding this comment.
I think what may have been wanted is
if (scale.X != 0 && scale.Y != 0)
scale.X = scale.Y = Math.Max(scale.X, scale.Y);
else if (scale.X != 0)
scale.Y = scale.X;
else if (scale.Y != 0)
scale.X = scale.Y;The misleading part about all this is that scale is all deltas, not the actual full final scale.
There was a problem hiding this comment.
I tried that initially but it doesn't work amazingly because as you say, it's arriving as deltas and therefore not in a stable state.
mThats why i chose to leave all the decisions based on the dragged anchor alone.
There was a problem hiding this comment.
Fair enough. I'll just kill the else branch and leave a TODO for now, then...
The previous implementation was checking if the `x0` or `x2` anchors
were selected to decide on which way to transfer the drawable's scale,
but that check actually ends up being always true for corner anchors. To
visualise, this is how the corner anchors correspond to `Anchor` flags:
x0 x1 x2
| | |
y0 -O---O---O-
| | |
y1 -O---+---O-
| | |
y2 -O---O---O-
| | |
The Os indicate where the reference anchors are on a selection box.
The first conditional eliminates the middle ones, which makes sense.
But after excluding them from further deliberations (marking via X):
x0 x1 x2
| | |
y0 -O---X---O-
| | |
y1 -X---+---X-
| | |
y2 -O---X---O-
| | |
The remaining anchors always have `x0` or `x2` set. So to avoid
confusion, just always transfer one way for now. At some point this
should be torn out in favour of an actual implementation of the desired
behaviour.
This should be considered temporary implementation along with the rest, intended to get the initial release into a roughly usable state. Please be easy on the review as it will get replaced with an abstracted version of
OsuSelectionHandlerin the very near future.