Summary: | SVGPathSegList needs better getTotalLength, getSegmentAtLength path traversal code | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | SVG | Assignee: | Dirk Schulze <krit> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | jeffschiller, krit, zimmermann | ||||
Priority: | P2 | ||||||
Version: | 420+ | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.4 | ||||||
Bug Depends on: | 10827, 44009 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Eric Seidel (no email)
2006-12-31 13:20:29 PST
I added these when working on animateMotion. However they don't yet work for general case SVGPathSegLists. Currently they only work on normalized/processed SVGPathSegLists which only contain lines, movetos, closepaths, and curvetos. Eventually support should be added for all the various types, including arcs (at least in the case of getSegmentAtLength). Also, right now SVGPathSegList has a couple copies of the same switch statement for doing these traversals. Better might be to add a length() method onto the individual segment classes, or to at least share the switch statement with a single common method (similar to how PathTraversalState is used to support sharing a single switch inside Path.cpp). We need a few JS test cases to demonstrate this. Normalized behavior of TraversalState shouldn't be a problem, once the patch on bug 44009 landed. Created attachment 94238 [details]
Patch
Comment on attachment 94238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94238&action=review > Source/WebCore/svg/SVGPathElement.cpp:67 > + SVGPathParserFactory* factory = SVGPathParserFactory::self(); I don’t think this local variable helps clarity or performance. > Source/WebCore/svg/SVGPathElement.cpp:75 > + SVGPathParserFactory* factory = SVGPathParserFactory::self(); I don’t think this local variable helps clarity or performance. > Source/WebCore/svg/SVGPathParserFactory.cpp:282 > + totalLength = builder->totalLength(); Should this be done only if ok is true? > Source/WebCore/svg/SVGPathParserFactory.cpp:299 > + point = builder->currentPoint(); Should this be done only if ok is true? > Source/WebCore/svg/SVGPathTraversalStateBuilder.cpp:88 > bool SVGPathTraversalStateBuilder::continueConsuming() > { > - ASSERT(m_traversalState); > - ASSERT(m_traversalState->m_action == PathTraversalState::TraversalSegmentAtLength); > - return m_traversalState->m_totalLength < m_traversalState->m_desiredLength; > + ASSERT(m_traversalState); > + if (m_traversalState->m_action == PathTraversalState::TraversalSegmentAtLength > + && m_traversalState->m_totalLength >= m_traversalState->m_desiredLength) > + m_traversalState->m_success = true; > + > + if ((m_traversalState->m_action == PathTraversalState::TraversalPointAtLength > + || m_traversalState->m_action == PathTraversalState::TraversalNormalAngleAtLength) > + && m_traversalState->m_totalLength >= m_traversalState->m_desiredLength) { > + FloatSize change = m_traversalState->m_current - m_traversalState->m_previous; > + float slope = atan2f(change.height(), change.width()); > + if (m_traversalState->m_action == PathTraversalState::TraversalPointAtLength) { > + float offset = m_traversalState->m_desiredLength - m_traversalState->m_totalLength; > + m_traversalState->m_current.move(offset * cosf(slope), offset * sinf(slope)); > + } else > + m_traversalState->m_normalAngle = rad2deg(slope); > + m_traversalState->m_success = true; > + } > + m_traversalState->m_previous = m_traversalState->m_current; > + > + return !m_traversalState->m_success; > } Seeing m_traversalState-> so many times over and over again leads to the question of whether m_traversalState could have some function members to make this code simpler. A function like this is a bit of an anti-pattern. Comment on attachment 94238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94238&action=review >> Source/WebCore/svg/SVGPathElement.cpp:67 >> + SVGPathParserFactory* factory = SVGPathParserFactory::self(); > > I don’t think this local variable helps clarity or performance. Fixed. >> Source/WebCore/svg/SVGPathElement.cpp:75 >> + SVGPathParserFactory* factory = SVGPathParserFactory::self(); > > I don’t think this local variable helps clarity or performance. Fixed. >> Source/WebCore/svg/SVGPathParserFactory.cpp:282 >> + totalLength = builder->totalLength(); > > Should this be done only if ok is true? It doesn't matter. The idea is, that you check the status before using totalLength(). And totalLength() should not cause an invalid behavior (also checked by an assert). But it is just possible to grab totalLength after the parsePathDataFromSource call. >> Source/WebCore/svg/SVGPathParserFactory.cpp:299 >> + point = builder->currentPoint(); > > Should this be done only if ok is true? Ditto. >> Source/WebCore/svg/SVGPathTraversalStateBuilder.cpp:88 >> } > > Seeing m_traversalState-> so many times over and over again leads to the question of whether m_traversalState could have some function members to make this code simpler. A function like this is a bit of an anti-pattern. I'll check this. Committed r86973: <http://trac.webkit.org/changeset/86973> Committed r86979: <http://trac.webkit.org/changeset/86979> |