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 the steps to find a point on an ellipse #8495

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

Kaiido
Copy link
Member

@Kaiido Kaiido commented Nov 11, 2022

This is an attempt at fixing #8408.

This goes the long way of defining the steps to determine the point on the ellipse, in a "theoretical" wording (as opposed to using algebra formulae).
This is based on what browsers seems to do, so there shouldn't be a need for browser issues, yet a technical review from @whatwg/canvas is probably necessary.
Not sure either if editorially this is the proper way of fixing that.

Ps: Also CCing @flupe since they were the ones discovering this in the first place.

(See WHATWG Working Mode: Changes for more details.)


/canvas.html ( diff )

This is an attempt at fixing whatwg#8408.
This goes the long way of defining the steps to determine the point on the ellipse, in a "theoretical" wording (as opposed to using algebra formulae).
This is based on what browsers seems to do.  so there shouldn't be a need for browser issues, yet a technical review from @whatwg/canvas is probably necessary.
Not sure either if editorially this is the proper way of fixing that.
@annevk
Copy link
Member

annevk commented Nov 11, 2022

cc @whatwg/canvas (pr-preview cannot mention teams so mentioning them in OP often fails)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Here's some minor editorial review... hopefully some domain experts can still chime in to help us with the review of the actual logic :)

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Jan 11, 2023

Anyone from @whatwg/canvas want to provide a quick check on the logic here? I'll plan on merging this by Monday the 16th if we don't hear otherwise.

@domenic domenic self-assigned this Jan 11, 2023
@fserb
Copy link
Contributor

fserb commented Jan 11, 2023

I think this makes sense. Thanks, @Kaiido for doing this. :)

@Kaiido
Copy link
Member Author

Kaiido commented Feb 24, 2023

Gentle ping @domenic, I guess this went off your radar ;-)

@domenic domenic merged commit 9170296 into whatwg:main Feb 24, 2023
teoli2003 added a commit to mdn/content that referenced this pull request Feb 24, 2023
* Fix ellipse angles definition

As was found in whatwg/html#8408 the ellipse's `startAngle` and `endAngle` are not really angles on the ellipse, but angles on the eccentric circle of the ellipse.   
These are known as "eccentric angle", though this term *might* be a bit obscure at first sight, so I wouldn't mind a better proposition if reviewers have one.  
The specs got fixed in whatwg/html#8495 but as can be seen, the fix was quite verbose and I'm not sure the docs need to get that much details.  
So I hope this little fix is both enough to lead the ones that could get surprised by the actual behavior to the proper understanding, and not too obscure to lose in details the ones that wouldn't notice.

* Link "eccentric angle" to outside explanation

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

* Should have batched...

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

---------

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants