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: Correctly calculate the bounds of hat blocks. #8616

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Oct 9, 2024

The basics

The details

Resolves

Fixes gonfunko/scratch-blocks#201

Proposed Changes

This PR updates the calculation of the height of hats on blocks. Previously, the height of a hat was the value that was used for the Y position of points on an SVG curve. However, this is not the actual Y position of e.g. the apex of the curve, but rather the Y position of the control points used to define the Bézier curve, which is different from the actual max Y value of the curve itself; note the position of the lower control points relative to the bottom of the curve in the graphic here: https://developer.mozilla.org/en-US/docs/Web/SVG/Tutorial/Paths#b%C3%A9zier_curves.

Per https://stackoverflow.com/a/5327329, the height of the curve is 3/4 the Y position of the control points, at least in the simple situation we use for hats. As a result, before this change, the bounds of hat blocks were incorrect:
Screenshot 2024-10-09 at 11 15 27 AM
Screenshot 2024-10-09 at 11 15 20 AM
Screenshot 2024-10-09 at 11 15 14 AM

With this change, the bounds are as expected across all three built-in renderers:
Screenshot 2024-10-09 at 11 14 23 AM
Screenshot 2024-10-09 at 11 14 30 AM
Screenshot 2024-10-09 at 11 14 37 AM

@gonfunko gonfunko requested a review from a team as a code owner October 9, 2024 18:23
@github-actions github-actions bot added the PR: fix Fixes a bug label Oct 9, 2024
@gonfunko gonfunko merged commit 9fc6931 into google:rc/v12.0.0 Oct 14, 2024
11 checks passed
@gonfunko gonfunko deleted the hat-bounds branch October 14, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants