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

Fix bug where close-to-collinear line segments with thickness >= 2 got size f64::INFINITTY as i32 as coordinates. #574

Merged
merged 6 commits into from
May 8, 2024

Conversation

el-hult
Copy link
Contributor

@el-hult el-hult commented Apr 23, 2024

Bugfix for #562 .

The root cause of the bug was a comparison of floats, but to make sure I got it right, I changed the code away from early returns (that was part of why the bug surfaced) and added some more comments.

This script shows example thick lines with inside and outside corners, and the capping feature.

use plotters::prelude::*;
pub fn main() -> Result<(), Box<dyn std::error::Error>> {
    let root = BitMapBackend::new("test.bmp", (1000, 1000)).into_drawing_area();
    let mut chart = ChartBuilder::on(&root)
        .build_cartesian_2d(0.0..1000.0, 0.0..1000.0)?;
    let pahts = vec![
        PathElement::new(
            vec![(336.0, 614.0), (339.0, 674.0),(341.0,714.0)],
            ShapeStyle::from(RED).stroke_width(2),
        ),
        PathElement::new(
            vec![(100.0, 100.0), (150.0, 150.0),(200.0,100.0)],
            ShapeStyle::from(BLUE).stroke_width(2),
        ),
        PathElement::new(
            vec![(400.0, 400.0), (400.0, 450.0),(400.0,500.0)],
            ShapeStyle::from(GREEN).stroke_width(5),
        ),
        PathElement::new(
            vec![(900.0, 410.0), (600.0, 400.0),(900.0,500.0), (1000.0, 1000.0)],
            ShapeStyle::from(YELLOW).stroke_width(10),
        ),
    ];
    chart.draw_series(pahts.into_iter())?;
    Ok(())
}

this code previously failed getting stuck in a very very long loop.
the example shows the pathological case (separately included as a unit test) but also some simple cases with with and without capping.
@el-hult
Copy link
Contributor Author

el-hult commented Apr 29, 2024

Hi @AaronErhardt You asked in #345 that we should ping you if the issue goes without notice. Time to take a look at this PR?

I have done what I hope is a quite minimal fix. I have created a unit test and an example that shows the output. Is okay?

@el-hult
Copy link
Contributor Author

el-hult commented May 8, 2024

@AaronErhardt I did revert a bit of the changes, to make the PR smaller and easier to review. Looks okay now?

Copy link
Member

@AaronErhardt AaronErhardt left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Sorry for the delay, I've been quite busy lately.

@AaronErhardt AaronErhardt merged commit 01cc98f into plotters-rs:master May 8, 2024
18 checks passed
@el-hult el-hult deleted the fix-562 branch May 14, 2024 13:57
@el-hult
Copy link
Contributor Author

el-hult commented May 14, 2024

Now I can go back to use the main plotters-rs instead of my fork. Thanks a lot!

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

2 participants