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

feat: add a simple paused and progress with event to control transition animation. #102

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

snowyu
Copy link

@snowyu snowyu commented Mar 11, 2020

No description provided.

Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

This is really great!

A few remarks:

  • Have you studied if there is any performance hit when animating hundreds of elements, sending progress events all the time (even when nothing listens to them?).

  • I was surprised in the following scenario (https://observablehq.com/d/97dc4e77951d416d), that calling transition.paused(false) did not seem to resume the transition at the progress where it was paused (about 1/4), but rather go directly to the end (note: I've built the feature on branch two)

  • Listening to on("progress") we don't have an easy access to elapsed or to progress; I'm not sure it's possible to do better, though.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/transition/progress.js Show resolved Hide resolved
test/transition/paused-test.js Outdated Show resolved Hide resolved
test/transition/paused-test.js Outdated Show resolved Hide resolved
test/transition/paused-test.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
snowyu and others added 6 commits July 9, 2020 08:55
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
@snowyu
Copy link
Author

snowyu commented Jul 9, 2020

  1. Performance minimize it depends on the time consumed by listeners. Nevertheless, I've still done the following optimization.: no events will be emitted when the progress is unchanged.

  2. I've found a strange error in your (https://observablehq.com/d/97dc4e77951d416d) scenario:

    • This shouldn't happen.
    VM32 observablehq-21:35 Uncaught (in promise) TypeError: transition.interrupt is not a function
     at eval (VM20 observablehq-21:35)
    
  3. the progress has been passed in the event(the fourth argument).

    .on("progress", function(data, index, group, progress) {
     });

@Fil
Copy link
Member

Fil commented Jul 9, 2020

Thanks for pointing out that progress is the 4th argument, I had missed it. And I've now fixed the transition.interrupt error in the test notebook.

I still don't understand why progress jumps from 0.25 to 1 when we resume the transition at 5000ms. It seems that it restarts at 0.5 if I resume at 3000ms, so it may be because the actual time elapsed during the pause has been counted somehow?

Also, I get an error when I click "replay" in the middle of the transition, I'm not sure why.

@snowyu
Copy link
Author

snowyu commented Jul 9, 2020

  1. I should add an unit test to verify it.
  2. It's seem that the transition is done(disposed). Sorry I do not know the observablehq.

@snowyu
Copy link
Author

snowyu commented Jul 9, 2020

finally I got it. You can try now.

@Fil
Copy link
Member

Fil commented Jul 9, 2020

perfect! https://observablehq.com/d/97dc4e77951d416d is working as expected (see https://observablehq.com/d/97dc4e77951d416d@59 for the previous version).
The other error I mentioned was because I had not properly disposed the timers, that's fixed too.

So the only question that remains for me is the possible performance hit. In the default case, when no listeners are set on "progress", can we make sure we're not spending more CPU cycles?

@snowyu
Copy link
Author

snowyu commented Jul 9, 2020

  • D3:
    d3

  • D3-Progress:
    d3-progress

@Fil Fil added the feature label Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants