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

Refactor SVG stroke width/bounds calculation #85

Closed

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Apr 23, 2019

Resolves

Resolves scratchfoundation/scratch-render#434
Resolves scratchfoundation/scratch-gui#4479

Proposed Changes (EDIT 2020-02-14)

This PR:

  1. Changes 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.
  2. Adds unit tests for SvgRenderer._findLargestStrokeWidth to 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).
  3. Fixes SvgRenderer._findLargestStrokeWidth to 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 a stroke-width, which was counted as expanding the costume's bounds. This has been properly fixed by changing SvgRenderer._findLargestStrokeWidth to match the SVG spec's definition of strokes.

@adroitwhiz adroitwhiz changed the title Always enlarge SVG bounds by largest stroke width, even if original width or height < 0 Always enlarge SVG bounds by largest stroke width, even if original width or height = 0 Apr 23, 2019
@thisandagain thisandagain added this to the May 2019 milestone Apr 29, 2019
@adroitwhiz
Copy link
Contributor Author

After more investigation, it looks like this is basically a revert of #73. This probably breaks other stuff.

@adroitwhiz
Copy link
Contributor Author

adroitwhiz commented May 16, 2019

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.

@towerofnix
Copy link

@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?

@adroitwhiz adroitwhiz force-pushed the always-enlarge-bounds branch 2 times, most recently from 6f81e0d to f53a0b3 Compare February 14, 2020 21:36
@adroitwhiz adroitwhiz changed the title Always enlarge SVG bounds by largest stroke width, even if original width or height = 0 Refactor SVG stroke width/bounds calculation Feb 14, 2020
@adroitwhiz
Copy link
Contributor Author

adroitwhiz commented Feb 14, 2020

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.

@adroitwhiz adroitwhiz requested a review from fsih February 14, 2020 21:44
@adroitwhiz adroitwhiz force-pushed the always-enlarge-bounds branch 2 times, most recently from df1693e to d454bec Compare February 27, 2020 03:42
@adroitwhiz
Copy link
Contributor Author

adroitwhiz commented Mar 5, 2020

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.

@apple502j
Copy link

@adroitwhiz
Copy link
Contributor Author

This depends on the stroke attribute being moved down to "leaf" items-- blocked on #143

@fsih fsih assigned fsih and unassigned paulkaplan May 27, 2020
@fsih
Copy link
Contributor

fsih commented May 27, 2020

When bug hunting, remember to check for #73 : delete the costume, and then use the pen tool

Copy link
Contributor

@fsih fsih left a 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!

@fsih
Copy link
Contributor

fsih commented Jun 4, 2020

The bug is magically fixed! (by paper.js bump?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants