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 Bezier Shapes #1120 #1178

Merged
merged 4 commits into from
Jan 31, 2022
Merged

Add Bezier Shapes #1120 #1178

merged 4 commits into from
Jan 31, 2022

Conversation

xudesheng
Copy link
Contributor

@xudesheng xudesheng commented Jan 29, 2022

Closes #1120.

This is a replacement of the PR: #1155 for two reasons:

  1. It's based on the latest master branch until noontime on Jan.29th, 2022. I can leverage PR Add ui.data(), ctx.data(), ctx.options() and ctx.tessellation_options() #1175 to manipulate the bezier_tolerance.
  2. better formatting.

@xudesheng xudesheng mentioned this pull request Jan 29, 2022
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks great! But there are still the clippy warnings to silence, and you need to run cargo fmt. If you make the PR collaborative I can help you fix it!

egui/src/introspection.rs Outdated Show resolved Hide resolved
egui_demo_lib/src/apps/demo/paint_bezier.rs Outdated Show resolved Hide resolved
epaint/src/bezier.rs Show resolved Hide resolved
@xudesheng
Copy link
Contributor Author

Thank you @emilk, I fixed all Clippy warnings. I learned more from this journey. 👍 Kindly please review it again and hopefully there is a better solution for the code between lines 64-68 in paint_bezier.rs to have a widget that can manipulate the tasseller options.

@xudesheng
Copy link
Contributor Author

Hi, @emilk Clippy is still reporting some error message but it seems like it is aginst the old commit. None of them exists in the current latest commit now. How shall I do the next step? sorry since this is almost my "first" PR. :-)

@emilk
Copy link
Owner

emilk commented Jan 31, 2022

Hi, @emilk Clippy is still reporting some error message but it seems like it is aginst the old commit. None of them exists in the current latest commit now. How shall I do the next step? sorry since this is almost my "first" PR. :-)

If you run cargo clippy --all-targets you should see a few issues, starting with:

❯ cargo clippy --all-targets
    Checking egui v0.16.1 (/Users/emilk/code/egui/egui)
    Checking web-sys v0.3.55
warning: calls to `push` immediately after creation
   --> epaint/src/bezier.rs:758:9
    |
758 | /         let mut result = Vec::new();
759 | |         result.push(curve.points[0]); //add the start point
    | |_____________________________________^ help: consider using the `vec![]` macro: `let mut result = vec![..];`
    |
note: the lint level is defined here
   --> epaint/src/lib.rs:12:5
    |
12  |     clippy::all,
    |     ^^^^^^^^^^^
    = note: `#[warn(clippy::vec_init_then_push)]` implied by `#[warn(clippy::all)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#vec_init_then_push



```

@xudesheng
Copy link
Contributor Author

cargo clippy --all-targets

Aha, all from the test code in the bezier.rs file. Thank you, I just fixed all of them and committed.

@emilk emilk changed the title Bezier Curve Enhancement for #1120 Add Bezier Shapes #1120 Jan 31, 2022
@emilk emilk merged commit 1f03f53 into emilk:master Jan 31, 2022
@emilk
Copy link
Owner

emilk commented Jan 31, 2022

Thank you!

@xudesheng xudesheng deleted the beziercurve1120 branch January 31, 2022 19:30
@4JX
Copy link
Contributor

4JX commented Feb 1, 2022

This PR introduces a typo, bezier_tolerence (Should be tolerance). It is present in tesellator.rs and introspection.rs.

@xudesheng
Copy link
Contributor Author

Thank you, @4JX , you are right. I can submit a new PR to fix them if @emilk thinks it's worth a dedicated PR.

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.

Bézier Curve Support as a new Plot item
4 participants