Skip to content

refactor: remove createjs.Tween dependency from render loop and tick handler#7470

Open
Mohd-Ali-Creator wants to merge 1 commit into
sugarlabs:masterfrom
Mohd-Ali-Creator:fix/remove-tween-dependency
Open

refactor: remove createjs.Tween dependency from render loop and tick handler#7470
Mohd-Ali-Creator wants to merge 1 commit into
sugarlabs:masterfrom
Mohd-Ali-Creator:fix/remove-tween-dependency

Conversation

@Mohd-Ali-Creator

@Mohd-Ali-Creator Mohd-Ali-Creator commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

PR Category

  • Bug Fix
  • Feature
  • Performance
  • Tests
  • Documentation

Summary

Removes createjs.Tween.hasActiveTweens() calls
from two locations as part of the CreateJS
migration effort (Issue #6612).

Changes

1. _startRenderLoop (line ~545)

Removed createjs.Tween.hasActiveTweens() check.
All render conditions already covered by:

  • this.stageDirty — visual changes
  • hasActiveGifs — GIF animations
  • isInteracting — drag/select states

2. __tick function (line ~5099)

Removed createjs.Tween.hasActiveTweens()
from condition. this.update flag is sufficient.

Why This Is Safe

All animation states covered by Tween tracking
are already handled by existing flags.
No behavioral change — removes CreateJS dependency.

Note on Jest Failure

The failing test (aidebugger.test.js) is a
pre-existing failure on master branch —
NOT introduced by this PR.

Verified by running test on clean master:
npx jest aidebugger.test.js → FAILS on master too

Reference: #7403 (raised by @kartikscodes)

cc @walterbender @sum2it

Testing

  • App loads correctly ✅
  • Block dragging works ✅
  • Turtle animations work ✅
  • Jest: 5383/5384 tests passing ✅
  • 1 pre-existing failure in aidebugger.test.js
    exists on master (unrelated to this change)

Part of CreateJS migration: #6612

cc @walterbender @sum2it @omsuneri

@github-actions github-actions Bot added performance Improves performance (load time, memory, rendering) size/S Small: 10-49 lines changed area/javascript Changes to JS source files labels Jun 4, 2026
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Coverage: Statements: 48.23% | Branches: 39.78% | Functions: 52.95% | Lines: 48.63%
Master Coverage: Statements: 48.29% | Branches: 39.83% | Functions: 52.98% | Lines: 48.69%

Note: These failures may be introduced by this PR or may already exist in the master branch.
Tip: Update your branch with the latest master and rerun tests.
If the same failures are present on master, they are likely not introduced by this PR.

Failed Tests:

aidebugger.test.js

@github-actions

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

@ssz2605

ssz2605 commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

@Mohd-Ali-Creator Rebase on to the master

@ssz2605

ssz2605 commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

@Mohd-Ali-Creator I noticed there are still active createjs.Tween.get(...) usages in artwork.js, block.js, and trash.js. Can you clarify why removing createjs.Tween.hasActiveTweens() is safe in the render loop? Are these tween animations guaranteed to update stageDirty/update so rendering continues while they are active?

@Mohd-Ali-Creator Mohd-Ali-Creator force-pushed the fix/remove-tween-dependency branch from d72589f to 9f6e286 Compare June 17, 2026 07:08
@github-actions github-actions Bot added size/XS Extra small: < 10 lines changed and removed size/S Small: 10-49 lines changed needs-rebase labels Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

✅ All Jest tests passed! This PR is ready to merge.

Coverage: Statements: 49.43% | Branches: 40.91% | Functions: 53.88% | Lines: 49.85%
Master Coverage: Statements: 49.43% | Branches: 40.91% | Functions: 53.88% | Lines: 49.84%

@Mohd-Ali-Creator

Copy link
Copy Markdown
Contributor Author

Hi @ssz2605!

Thank you for the detailed review as
this is a valid concern.

You are correct that createjs.Tween.get()
is still actively used in artwork.js,
block.js, and trash.js for animations
like block highlighting.

After your feedback, I investigated
whether stageDirty is set during Tween
animation frames. You are right to flag
this —> if Tween animations run without
triggering stageDirty updates, removing
hasActiveTweens() from the render loop
could cause animations to freeze
mid-execution.

This removal may be premature while
createjs.Tween.get() is still in use
across the codebase.

Two options I see:

  1. Revert this change until Tween itself
    is fully migrated away
  2. Add stageDirty = true into Tween tick
    callbacks to maintain rendering

@sum2it @walterbender — which approach
do you prefer for the migration plan?

Also rebased on latest master
:)!

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

@ssz2605

ssz2605 commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

@Mohd-Ali-Creator Please resolve merge conflicts.

@Mohd-Ali-Creator Mohd-Ali-Creator force-pushed the fix/remove-tween-dependency branch from 9f6e286 to 7e0ac1a Compare July 12, 2026 20:31
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.27%. Comparing base (e58db21) to head (7e0ac1a).

Files with missing lines Patch % Lines
js/activity.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7470   +/-   ##
=======================================
  Coverage   59.27%   59.27%           
=======================================
  Files         176      176           
  Lines       57975    57974    -1     
=======================================
  Hits        34365    34365           
+ Misses      23610    23609    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files performance Improves performance (load time, memory, rendering) size/XS Extra small: < 10 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

2 participants