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

Triangles #86

Merged
merged 18 commits into from
May 5, 2019
Merged

Triangles #86

merged 18 commits into from
May 5, 2019

Conversation

weichweich
Copy link
Contributor

I added an iterator which can draw triangles. It uses the line iterator, to draw the edges. To fill the triangle I start to draw the edges at the top left corner and fill the space between the points on the edge.

Because the current way of drawing lines not always starts drawing at the start point, I had to reimplement the line drawing algorithm.

There are some issues with my triangle implementation. First issue is that triangles have no stroke width. The second issue is that the triangle iterator returns some points multiple times. The corner point e.g. are return at least twice because they are part of two edges which are drawn separately.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've left a preliminary review, and I'll test the simulator examples when I'm next at a Linux machine. Can you add a Triangle bullet to the Primitives list on the docs homepage please?

If a line has zero length, surely it shouldn't be drawn at all? What's the current behaviour in Embedded Graphics for this? Is there an established/expected behaviour from the wider graphics community?

Don't worry about stroke width - that's something that needs to be fixed for the line primitive as well!

d,
s,
err: d[0] + d[1],
e2: 0,
Copy link
Member

Choose a reason for hiding this comment

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

I think these single letters are from Bresenham's algorithm, no? Can you give d, s and e2 more descriptive names please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed now

embedded-graphics/src/primitives/line.rs Show resolved Hide resolved
embedded-graphics/src/primitives/line.rs Show resolved Hide resolved
simulator/examples/fill.rs Show resolved Hide resolved
@@ -107,7 +107,7 @@ pub struct DisplayBuilder {
impl DisplayBuilder {
pub fn new() -> Self {
Self {
width: 256,
width: 320,
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added triangles to the stroke example and wanted to make them fit into the window. I didn't realize that this should be done in the example itself. Should be fixed with my last commit.

@weichweich
Copy link
Contributor Author

Thanks for the PR! I've left a preliminary review, and I'll test the simulator examples when I'm next at a Linux machine. Can you add a Triangle bullet to the Primitives list on the docs homepage please?

Done

If a line has zero length, surely it shouldn't be drawn at all? What's the current behaviour in Embedded Graphics for this? Is there an established/expected behaviour from the wider graphics community?

I didn't put much thought in that. I think the behaviour should not be changed. So zero length lines should no longer be drawn.

Don't worry about stroke width - that's something that needs to be fixed for the line primitive as well!

Lines with a stroke width are rectangles. If the triangle solution is stable enough, it could be reused for lines and polygons.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Tested in the simulator. This looks awesome! Thanks for spending the time implementing this feature for embedded_graphics 🙂

@jamwaffles jamwaffles merged commit f2e105d into embedded-graphics:master May 5, 2019
@jamwaffles
Copy link
Member

Released in 0.4.8.

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