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

Arc end_angle should be renamed to span_angle #400

Closed
BillyDM opened this issue Jun 10, 2020 · 3 comments · Fixed by #401
Closed

Arc end_angle should be renamed to span_angle #400

BillyDM opened this issue Jun 10, 2020 · 3 comments · Fixed by #401
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@BillyDM
Copy link

BillyDM commented Jun 10, 2020

After scratching my head for a while, I found out that the end_angle field in iced::widget::canvas::path::arc::Arc is not the angle where the end of the arc lies. It is actually the span of the arc.

In other words, the end of the arc lies at start_angle + end_angle. The field end_angle should be renamed to something like span_angle to prevent confusion.

https://docs.rs/iced/0.1.1/iced/widget/canvas/path/struct.Arc.html

@hecrj
Copy link
Member

hecrj commented Jun 10, 2020

We should keep the name unchanged and fix the Frame implementation instead.

@hecrj hecrj added the bug Something isn't working label Jun 10, 2020
@hecrj hecrj added this to the 0.2.0 milestone Jun 10, 2020
@hecrj hecrj added the good first issue Good for newcomers label Jun 10, 2020
@Vanille-N
Copy link
Contributor

If I may, I believe it's as simple as

// graphics/src/widget/canvas/path/builder.rs
@@ -84,7 +84,7 @@ impl Builder
- sweep_angle: math::Angle::radians(arc.end_angle),
+ sweep_angle: math::Angle::radians(arc.end_angle - arc.start_angle),

I've checked that all other uses of end_angle (2 in total) are correct:

// graphics/src/widget/canvas/path/builder.rs
impl From<Arc> for Elliptical {
    fn from(arc: Arc) -> Elliptical {
        Elliptical {
            // fields omitted
            end_angle: arc.end_angle,   // line 41
        }
    }
}

and miraculously, the one other place where end_angle is used also needs no change (which perhaps explains why it went unnoticed)

// graphics/src/widget/canvas/path/builder.rs
#[inline]
pub fn circle(&mut self, center: Point, radius: f32) {
    self.arc(Arc {
        center,
        radius,
        start_angle: 0.0,
        end_angle: 2.0 * std::f32::consts::PI   // line 157
    });
}

Seeing that this is labeled good first issue, I assume it's okay for a beginner like me to open a pull request ?

@hecrj
Copy link
Member

hecrj commented Jun 11, 2020

@Vanille-N Sounds good! By all means, feel free to open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants