Skip to content

fix reverse animation play and improve code structure #5028

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

mohammadbaghaei
Copy link
Contributor

No description provided.

@mohammadbaghaei
Copy link
Contributor Author

mohammadbaghaei commented Apr 16, 2025

Also, I’d appreciate it if you could take a look at the GitHub Pages deployment for modelviewer.dev. It seems that new changes aren’t being deployed properly due to a token or permission issue.

@samaneh-kazemi
Copy link
Collaborator

Also, I’d appreciate it if you could take a look at the GitHub Pages deployment for modelviewer.dev. It seems that new changes aren’t being deployed properly due to a token or permission issue.

Thanks for your patience. We’re actively reviewing what’s changed since the previous handoff and will update you shortly.

@samaneh-kazemi
Copy link
Collaborator

Hi, thanks for contributing to model-viewer. I noticed each of your commits address a different topic. I'd recommend to have separate PRs for each of those. Having separate PRs make it easier to review, test, and revert them individually.

@mohammadbaghaei
Copy link
Contributor Author

Hi, thanks for contributing to model-viewer. I noticed each of your commits address a different topic. I'd recommend to have separate PRs for each of those. Having separate PRs make it easier to review, test, and revert them individually.

Hi! Thanks for pointing that out — it's a great catch. I'll definitely keep it in mind for future contributions.
That said, I feel the overall review and PR merging process could be a bit faster. It would really help with keeping things moving smoothly.

@davide-granello
Copy link

davide-granello commented Jul 4, 2025

Hello @mohammadbaghaei i had the same issue as #5026 so i've tried using your fork but i still have the same issue.
When i play an animation backward, it seems the animation skips directly to the end (so the start of the forward animation).

This is how i play it forward and backward using a condition:

modelViewer.appendAnimation(animation.name, {repetitions: 1, timeScale: playForward ? 1 : -1})

@samaneh-kazemi
Copy link
Collaborator

Hi @mohammadbaghaei, thanks for your thoughtful reply and for being open to feedback. I completely understand how a faster review and merging process can make a big difference for contributors, and I appreciate your patience.

This PR addresses both the reverse animation fix and some broader code structure improvements. To help with review and make merging easier, would you be able to split this into smaller PRs? For example, one PR could focus solely on fixing the reverse animation play issue, and another on the code refactoring. This way, each change can be reviewed and tested independently, which reduces risk and usually helps things move through the review process more quickly.

Also, when you’ve updated the PRs or have new ones ready for review, please feel free to ping me, @-mention me, or bump the thread so I know you’re ready for feedback. I’ll do my best to prioritize the review.

Thanks again for your contributions and for sharing your perspective!

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

Successfully merging this pull request may close these issues.

None yet

3 participants