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

Replace line_drawing with clipline #381

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Replace line_drawing with clipline #381

merged 2 commits into from
Oct 31, 2023

Conversation

nxsaken
Copy link
Contributor

@nxsaken nxsaken commented Oct 29, 2023

Replaced line_drawing with my clipline crate which implements a variation of Bresenham's algorithm with built-in pixel-perfect clipping.

@nxsaken nxsaken changed the title Replaced line_drawing with clipline Replace line_drawing with clipline Oct 29, 2023
@parasyte
Copy link
Owner

Does this crate offer better performance or something? It has no dependencies, which might be enough of an improvement to make this change worth it.

But I would strongly prefer an external iterator interface over the callback. I have written a little about why in another issue. There really isn't any reason to not let the caller write the loop. And enforcing an internal iteration is a form of inversion of control which I am firmly against on principle.

If you switch it to external iteration, I don't see any reason not to make the change. Assuming it doesn't cause performance regressions or anything.

@nxsaken
Copy link
Contributor Author

nxsaken commented Oct 30, 2023

Yes, this crate clips the line before the tight loop (in constant time), allowing you to skip the bounds checks when indexing into the pixel buffer. It also skips Bresenham's algorithm for vertical and horizontal lines. I haven't had experience writing non-trivial Iterators, so I avoided doing it for this crate - but I'll look into it. Thank you for sharing the discussion and the talk!

@nxsaken nxsaken mentioned this pull request Oct 30, 2023
2 tasks
@nxsaken
Copy link
Contributor Author

nxsaken commented Oct 30, 2023

I implemented the iterator(s) and updated the examples. You can either iterate over the Clipline enum itself, or match on it and move the loop(s) inside its arms, which will remove the overhead of the match from every iteration. I decided not to do that to keep the examples concise.

@parasyte
Copy link
Owner

That looks very good! Thank you for making that change. I know it's a lot to ask, but I also believe it is a marked improvement for usability.

I'll let the CI run, and it should inform you if there is anything that needs to be addressed.

@nxsaken
Copy link
Contributor Author

nxsaken commented Oct 30, 2023

Thank you for your feedback, that was valuable! It's my first crate, so I'm extra grateful for the opportunity to learn :)

Copy link
Owner

@parasyte parasyte left a comment

Choose a reason for hiding this comment

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

All looks good! This can be merged now.

@parasyte parasyte merged commit 5461133 into parasyte:main Oct 31, 2023
9 checks passed
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.

2 participants