-
Notifications
You must be signed in to change notification settings - Fork 92
Refactor SVG stroke width/bounds calculation #85
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
Refactor SVG stroke width/bounds calculation #85
Conversation
|
After more investigation, it looks like this is basically a revert of #73. This probably breaks other stuff. |
|
Alright, I changed _findLargestStrokeWidth to not take stroke width into account if stroke="none". This should avoid a regression on scratchfoundation/scratch-gui#4233 while also fixing the two issues mentioned above. |
|
@adroitwhiz Okay I was going to suggest a test case be made so that this guaranteedly doesn't break that, but.. I have no idea how you'd make a test case for the VM within the SVG renderer repo. Still, figuring out some automatic way to insure that isn't broken would be good, perhaps? |
6f81e0d to
f53a0b3
Compare
|
This has been refactored to be more thorough/correct, cover more edge cases, and include tests. I've seen people complain about this very frequently on the Bugs and Glitches forum--will try to find links. |
df1693e to
d454bec
Compare
d454bec to
64d9ca9
Compare
|
Before testing to ensure this doesn't regress scratchfoundation/scratch-gui#4233, merge scratchfoundation/scratch-render#561 -- there are some existing rotation-center-related issues in the codebase that may interfere with testing. |
|
This depends on the |
|
When bug hunting, remember to check for #73 : delete the costume, and then use the pen tool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good once we merge STROKABLE_ELEMENTS and GRAPHICAL_ELEMENTS, and I pull it down to test it a bit!
|
The bug is magically fixed! (by paper.js bump?) |
Resolves
Resolves scratchfoundation/scratch-render#434
Resolves scratchfoundation/scratch-gui#4479
Proposed Changes (EDIT 2020-02-14)
This PR:
SvgRenderer._transformMeasurements()to always enlarge the SVG's bounding box by the largest stroke width in the document, even if the unenlarged bounds' width or height are 0.SvgRenderer._findLargestStrokeWidthto ensure that it treats stroke attributes the same way as the SVG specification (and ensures that sprites like the one from Doesn't work the block go to position. scratch-gui#4233 don't regress).SvgRenderer._findLargestStrokeWidthto actually pass those tests.Reason for Changes
There are certain scenarios (e.g. a perfectly vertical or horizontal line) in which one of the SVG's bounding box dimensions is 0 but the SVG should still have a size.
The previous code (introduced in #73) hid the underlying issue: an empty costume was given incorrect bounds because an empty costume's
<g>element had astroke-width, which was counted as expanding the costume's bounds. This has been properly fixed by changingSvgRenderer._findLargestStrokeWidthto match the SVG spec's definition of strokes.