Skip to content

[SVG] Refactoring accumulate logic for animateMotion #48748

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.

Sign up for GitHub

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

Jump to bottom
Closed
Ahmad-S792 wants to merge 1 commit into from
Closed

[SVG] Refactoring accumulate logic for animateMotion #48748

Ahmad-S792 wants to merge 1 commit into from

Conversation

@Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Jul 30, 2025 •
edited by webkit-early-warning-system
Loading

9e9fd42

[SVG] Refactoring accumulate logic for animateMotion
https://bugs.webkit.org/show_bug.cgi?id=296723

Reviewed by NOBODY (OOPS!).

Inspired by: https://chromium.googlesource.com/chromium/src/+/629e8ce3ef3deaf9b8fe705aba73061f7b2caa05

This removes helper function `buildTransformForProgress`, which only had
one use case and inlines the logic near call site. This patch is just
simpification of accumulate logic and paves way for future work.

* Source/WebCore/svg/SVGAnimateMotionElement.cpp:
(WebCore::SVGAnimateMotionElement::calculateAnimatedValue):
(WebCore::SVGAnimateMotionElement::buildTransformForProgress): Deleted.
* Source/WebCore/svg/SVGAnimateMotionElement.h:

9e9fd42

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 self-assigned this Jul 30, 2025
@Ahmad-S792 Ahmad-S792 added the SVG For bugs in the SVG implementation. label Jul 30, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3d56fa8)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the request-merge-queue Request a pull request to be added to merge-queue once ready label Jul 31, 2025
smfr
smfr reviewed Jul 31, 2025
View reviewed changes
Source/WebCore/svg/SVGAnimateMotionElement.cpp Outdated
buildTransformForProgress(transform, 1);
for (unsigned i = 0; i < repeatCount; ++i) {
float positionAtEndOfDuration = m_animationPath.length() * 1;
auto endTraversalState = m_animationPath.traversalStateAtLength(positionAtEndOfDuration);
Copy link
Contributor

@smfr smfr Jul 31, 2025

Choose a reason for hiding this comment

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

None of the inputs to m_animationPath.traversalStateAtLength() depend on i, so isn't this repeating identical work each time through the loop?

Copy link
Contributor Author

@Ahmad-S792 Ahmad-S792 Jul 31, 2025

Choose a reason for hiding this comment

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

@smfr - good spot! now removed it, I think forgot to tackle it in first go.

@Ahmad-S792
[SVG] Refactoring accumulate logic for animateMotion
9e9fd42 
https://bugs.webkit.org/show_bug.cgi?id=296723

Reviewed by NOBODY (OOPS!).

Inspired by: https://chromium.googlesource.com/chromium/src/+/629e8ce3ef3deaf9b8fe705aba73061f7b2caa05

This removes helper function `buildTransformForProgress`, which only had
one use case and inlines the logic near call site. This patch is just
simpification of accumulate logic and paves way for future work.

* Source/WebCore/svg/SVGAnimateMotionElement.cpp:
(WebCore::SVGAnimateMotionElement::calculateAnimatedValue):
(WebCore::SVGAnimateMotionElement::buildTransformForProgress): Deleted.
* Source/WebCore/svg/SVGAnimateMotionElement.h:
@Ahmad-S792 Ahmad-S792 force-pushed the eng/SVG-Refactoring-accumulate-logic-for-animateMotion branch from 3d56fa8 to 9e9fd42 Compare July 31, 2025 04:34
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 31, 2025 •
edited
Loading

EWS run on current version of this PR (hash 9e9fd42)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

shallawa
shallawa reviewed Jul 31, 2025
View reviewed changes
Source/WebCore/svg/SVGAnimateMotionElement.cpp
return true;
}

void SVGAnimateMotionElement::buildTransformForProgress(AffineTransform* transform, float percentage)
Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because you deleted this function, the code is duplicated below. Please keep this method and add a new parameter unsigned repeatCount to it and make it return PathTraversalState.

Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively you can split this into two functions:

static PathTraversalState traversalStateAtPositionOnPath(const Path& path, float percentage)
{

}

static void buildTransformForProgress(AffineTransform& transform, const PathTraversalState& traversalState, unsigned repeatCount)
{

}

Source/WebCore/svg/SVGAnimateMotionElement.cpp
Comment on lines -229 to +222
buildTransformForProgress(transform, percentage);
ASSERT(!m_animationPath.isEmpty());
float positionOnPath = m_animationPath.length() * percentage;
auto traversalState = m_animationPath.traversalStateAtLength(positionOnPath);
if (!traversalState.success())
return;
FloatPoint position = traversalState.current();
transform->translate(position);
Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You can just call

auto traversalState = buildTransformForProgress(*transform, percentage, 1);

Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With two separate functions, you can alternatively call:

auto traversalState = traversalStateAtPositionOnPath(m_animationPath, percentage);
buildTransformForProgress(*transform, traversalState, 1);

Source/WebCore/svg/SVGAnimateMotionElement.cpp
Comment on lines -233 to +231
for (unsigned i = 0; i < repeatCount; ++i)
buildTransformForProgress(transform, 1);
float pathLength = m_animationPath.length();
auto endTraversalState = m_animationPath.traversalStateAtLength(pathLength);
if (endTraversalState.success()) {
FloatPoint endPoint = endTraversalState.current();
transform->translate(endPoint.x() * repeatCount, endPoint.y() * repeatCount);
}
Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You can just call

buildTransformForProgress(*transform, 1, repeatCount);

Copy link
Contributor

@shallawa shallawa Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With two separate functions, you can alternatively call:

buildTransformForProgress(*transform, traversalStateAtPositionOnPath(m_animationPath, 1), repeatCount);

@Ahmad-S792 Ahmad-S792 removed the request-merge-queue Request a pull request to be added to merge-queue once ready label Aug 24, 2025
@Ahmad-S792
Copy link
Contributor Author

Ahmad-S792 commented Aug 24, 2025

I was trying to do it to solve - https://bugs.webkit.org/show_bug.cgi?id=257907 but I manage to fix the bug without needing this. So closing this PR.

@Ahmad-S792 Ahmad-S792 closed this Aug 24, 2025
@Ahmad-S792 Ahmad-S792 deleted the eng/SVG-Refactoring-accumulate-logic-for-animateMotion branch August 24, 2025 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smfr smfr smfr left review comments

@shallawa shallawa shallawa left review comments

@graouts graouts Awaiting requested review from graouts

@rwlbuis rwlbuis Awaiting requested review from rwlbuis

@nikolaszimmermann nikolaszimmermann Awaiting requested review from nikolaszimmermann

Assignees

@Ahmad-S792 Ahmad-S792

Labels

SVG For bugs in the SVG implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Ahmad-S792 @webkit-early-warning-system @smfr @shallawa