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

Add arithmetic between Points and f64s. #133

Merged
merged 1 commit into from Sep 22, 2020

Conversation

derekdreery
Copy link
Collaborator

This PR adds some convenience functions for adding (f64, f64) to a point.

Potential reasons not to do this: less type safety than using Vec2? I don't think this is an issue, but other opinions may differ.

@cmyr
Copy link
Member

cmyr commented Sep 19, 2020

I think my preference is to only have the arithmetic operations on the actual types? I appreciate it is unlikely to be a concern in practice, but by having to construct a Vec2 first the operation is explicit. Point + Size or Point + Point are meaningless, but Size + Size isn't; If we also impl these here we're saying that (f64, f64) can be a vec in some circumstances and a size in another.

For a similar reason, we don't implement From<Size> for Vec2 or From<Vec2> for Point; we want to force explicit casts, to ensure the user doesn't pass a Vec2 to some method that takes an impl Into<Point> argument.

So yea, on some vague conceptual level this makes me uneasy.

I'm curious about the case that motivated this? Are you wanting to avoid bringing Vec2 into scope?

@raphlinus
Copy link
Contributor

I'm also on the fence, but maybe a little more sympathetic. In my own work, I found myself not infrequently casting back and forth between Point and Vec2, and honestly it often felt more like type theater than accurately capturing the underlying concepts. To my mind, the intent of these methods is fairly clear, and it's certainly less noise than adding an additional Vec2::new() or what not. I'm also curious about the motivation, but feel I can be convinced.

@derekdreery
Copy link
Collaborator Author

derekdreery commented Sep 20, 2020

For context: here is the function where I would use the impl

/// Paint the actual physical fader that you move up and down.
fn fader(bounds: Rect, fg_brush: &Brush, bg_brush: &Brush, ctx: &mut PaintCtx) {
    const MIDDLE_LINE_BORDER: f64 = SLIDER_HEIGHT * 0.1;
    let middle_y = bounds.center().y;
    let center_line = Line::new(
        (bounds.x0 + MIDDLE_LINE_BORDER, middle_y),
        (bounds.x1 - MIDDLE_LINE_BORDER, middle_y),
    );

    // This shape is essentially a rectangle, but with a little bit of concave curve on the long sides.
    // TODO make this function independent of position (so that the path can be cached)
    const CURVE_FACTOR: f64 = 0.15;
    let shape = vec![
        PathEl::MoveTo(Point::new(bounds.x0, bounds.y0)),
        PathEl::QuadTo(
            bounds.center() - Vec2::new(0.0, bounds.height() * (0.5 - CURVE_FACTOR)),
            Point::new(bounds.x1, bounds.y0),
        ),
        PathEl::LineTo(Point::new(bounds.x1, bounds.y1)),
        PathEl::QuadTo(
            bounds.center() + Vec2::new(0.0, bounds.height() * (0.5 - CURVE_FACTOR)),
            Point::new(bounds.x0, bounds.y1),
        ),
        PathEl::ClosePath,
    ];

    ctx.fill(&*shape, bg_brush);
    ctx.stroke(&*shape, fg_brush, 1.5);
    ctx.stroke(center_line, fg_brush, 2.0);
}

The change would be purely aesthetic and would remove the Vec2::new tokens. You wouldn't be able to do it the other way round: the left operand must be a Point. The argument for is that it reduces noise whilst maintaining or improving description of intent.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

On balance, I think this is an improvement in readability. Thanks!

(do you need commit privs?)

@derekdreery
Copy link
Collaborator Author

I don't have commit privs here, just druid atm.

@derekdreery
Copy link
Collaborator Author

I think the key argument is that because you have chosen a Point as the left operand, you know you are applying a delta to it - it can't really be mis-construed as anything else.

@derekdreery derekdreery merged commit 6e2367c into linebender:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants