Add ellipse routines. Fix #146 #365
Conversation
|
For integer overflows, just say in the documentation what the maximum radius can be. |
| * Draws an unclipped ellipse. | ||
| * | ||
| * This is measured from the top left origin of the screen. | ||
| * Performs faster than using gfx_FillEllipse, but can cause corruption if used outside the bounds of the screen. |
| * @param a The horizontal radius of the ellipse. | ||
| * @param b The vertical radius of the ellipse. | ||
| */ | ||
| void gfx_FillEllipse_NoClip(int x, int y, int a, int b); |
There was a problem hiding this comment.
Please use the following types: uint24_t x, uint24_t y, uint8_t a, uint8_t b.
Also applies to gfx_Ellipse_NoClip.
There was a problem hiding this comment.
Please use the following types:
uint24_t x, uint24_t y, uint8_t a, uint8_t b.Also applies to
gfx_Ellipse_NoClip.
Pretty sure the radii should be uint24_t?
There was a problem hiding this comment.
I checked the implementation beforehand. It only reads the radii as one byte values.
There was a problem hiding this comment.
I don't see a reason why the functions can't just use uint24_t and functionality can be added later. Restricting it now in the prototype doesn't seem like a good idea.
There was a problem hiding this comment.
Hmm... I guess that's true. It's probably a bit better that the radii be uint24_t then.
| * @param a The horizontal radius of the ellipse. | ||
| * @param b The vertical radius of the ellipse. | ||
| */ | ||
| void gfx_FillEllipse(int x, int y, int a, int b); |
There was a problem hiding this comment.
Please use the following types: int24_t x, int24_t y, uint8_t a, uint8_t b.
Also applies to gfx_Ellipse.
There was a problem hiding this comment.
Please use the following types:
int24_t x, int24_t y, uint8_t a, uint8_t b.Also applies to
gfx_Ellipse.
Pretty sure the radii should be uint24_t?
|
Note for PT_: When I say that the fact that only radii of less than 128 is trash, it is just that - I am not referencing your code, I truly appreciate the pull request and think it is well done, and I am only saying that the minor issue that it doesn't support large radii is just a defect that will have to be resolved at one point because users will want it in the future. I'm not being personal about it, and I regret pointing it out, I should have handled that better. Anyway, the rest of it looks good so it can be merged and functionality added later if no one else minds. |
|
I have merged this PR in manually - I left the radii as |
No description provided.