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

[Bugfix] qp_ellipse overflow #19005

Merged
merged 8 commits into from
Oct 7, 2023
Merged

[Bugfix] qp_ellipse overflow #19005

merged 8 commits into from
Oct 7, 2023

Conversation

elpekenin
Copy link
Contributor

@elpekenin elpekenin commented Nov 8, 2022

Description

Not really sure if i should add the Bugfix and/or Core tags, so I'm not doing it.

Noticed that qp_ellipse does some weird drawing when somewhat-big inputs are provided. I saw this while testing my custom code to expose Quantum Painter over XAP (there might be an issue on my part too 😅).

To fix it, I've refactored the function using the mid-point ellipse algorithm, I haven't compared performance but I optimized the best that I could (no floats , reduced expressions, pre-compute values that are used all over the code).

During tests I noticed that it draws significantly worse when sizey is smaller than sizex, so a check was added to flip values if needed and it draws pretty nice now.

Note: The "tips" of the ellipse get trimmed as the sizes become bigger and the ratio between them is rather small: a cutoff line appears.

Above text is now updated, ended up re-doing the algorithm from scratch based on the parametric equation for an ellipse, using lib8tion for the trigonometric functions.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Nov 8, 2022
@drashna drashna requested a review from a team November 9, 2022 17:47
@github-actions
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jan 27, 2023
@tzarc tzarc added breaking_change_2023q4 and removed stale Issues or pull requests that have become inactive without resolution. labels Aug 18, 2023
@elpekenin
Copy link
Contributor Author

Re-reading this many months later i feel like i should have just done return false; if the helper function returns false...

@tzarc
Copy link
Member

tzarc commented Sep 25, 2023

There doesn't seem to be a copyright notice for the code in the linked site? Not sure we can accept it from a GPL perspective.

@elpekenin
Copy link
Contributor Author

Oh, right.. Didnt think about it back in the day.

Just made minimal searching and found no info about its licensing, guess i will look for another impl that has a clear (and compatible) license and test with that.

@elpekenin
Copy link
Contributor Author

elpekenin commented Sep 25, 2023

This draws much better than what i remember my first reimplementation doing. But I must say I'm not usually drawing ellipses...

I dont have any metrics, but it definitely is slightly slower than the current code in the repo. I'm fine personally with the tradeoff, given the current code often draws very funny shapes, and this code is much simpler.

@elpekenin elpekenin changed the title Improve algorithm for drawing ellipses [Enhancement] Better algorithm for qp_ellipse Sep 26, 2023
@elpekenin
Copy link
Contributor Author

For illustration on how it becomes very worse with size (crappy test code here)

canvas_30
canvas_110

@daskygit
Copy link
Member

out of interest why rewrite it rather than just fixing the overflow?

@daskygit
Copy link
Member

old implementation with the obvious overflows fixed, might have missed an edge case
image

new algorithm
image

At least to me the fixed old one looks better than the new algorithm.

@elpekenin
Copy link
Contributor Author

out of interest why rewrite it rather than just fixing the overflow?

Just because i didnt know it was an overflow 🤣😅

@elpekenin elpekenin changed the title [Enhancement] Better algorithm for qp_ellipse [Bugfix] qp_ellipse overflow Sep 28, 2023
@tzarc tzarc merged commit 21389fb into qmk:develop Oct 7, 2023
4 checks passed
@elpekenin elpekenin deleted the ellipse branch October 7, 2023 23:31
christrotter pushed a commit to christrotter/qmk_firmware that referenced this pull request Nov 28, 2023
zgagnon pushed a commit to zgagnon/qmk_firmware_waterfowl that referenced this pull request Dec 15, 2023
future-figs pushed a commit to future-figs/qmk_firmware that referenced this pull request Dec 27, 2023
mute-civilian pushed a commit to mute-civilian/qmk_firmware that referenced this pull request Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants