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 & improve line drawing code #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

udoschneider
Copy link

Fixes #13, #14

Replace general coordinate sorting with specific sorting for line_h/line_v.

Replace generic x-based line drawing with standard Bresenham drawing
which provides better results in all octants.

Fixes pappersverk#13, pappersverk#14

Replace general coordinate sorting with specific sorting for line_h/line_v.

Replace generic x-based line drawing with standard Bresenham drawing
which provides better results in all octants.
@PhillippOhlandt
Copy link
Collaborator

PhillippOhlandt commented Jun 18, 2022

Hey, could you maybe provide some example images for the before and after? And is the new implementation faster, slower or equal?

EDIT: There are some before/after examples in the failed tests and I don't really like the look of the new implementation. Take a look at the X example. The new version looks broken.

@udoschneider
Copy link
Author

I tried to make some visual comparisons here: https://github.com/udoschneider/oled_comparison

I'll check the X drawings ASAP.

However IMHO the bad quality of nearly vertical lines is typical for implementations which only use the x-Axis as a running variable to calculate the y position. You're basically ignoring the error buildup for y.

That's what Bresenham's Algorithm implemented in the "fix" takes care of.

Also after some thought I'm really not sure if any (more) drawing operations should be implemented in oled at all. IMHO oled provides the framebuffer. The task of (correctly) drawing are implemented by dedicated packages.

@PhillippOhlandt
Copy link
Collaborator

I think, in the end @luisgabrielroldan has to decide. Your comparison looks fine to me.

Also after some thought I'm really not sure if any (more) drawing operations should be implemented in oled at all. IMHO oled provides the framebuffer. The task of (correctly) drawing are implemented by dedicated packages.

The package provides enough primitives so that users can implement their own drawing. So I don't think we need to add new ones. But the existing ones should stay.

@luisgabrielroldan
Copy link
Collaborator

Hey! The fix looks pretty good! Thank you @udoschneider.
If you can fix the broken tests I agree to merge these changes.

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.

Vertical lines are not drawn if y2<y1
3 participants