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

✨ Added optional onDraw method to play to run after the _render call. #266

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

abidjappie
Copy link
Contributor

Description

Implements an optional callback that gets run after every Movie._render()

resolves: #230

Copy link
Collaborator

@clabe45 clabe45 left a comment

Choose a reason for hiding this comment

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

Hey Abid, thanks so much for contributing. Sorry for the delay, got a bit sidetracked with other stuff. The onDraw method should run every visual frame (each time we call _render to draw the frame onto the canvas).

@abidjappie abidjappie changed the title ✨ Added optional onDraw method to run after the _render call. ✨ Added optional onDraw method to play to run after the _render call. Mar 3, 2024
@abidjappie
Copy link
Contributor Author

Hey Abid, thanks so much for contributing. Sorry for the delay, got a bit sidetracked with other stuff. The onDraw method should run every visual frame (each time we call _render to draw the frame onto the canvas).

I updated the logic, let me know if I'm on the right track. 🙇

Comment on lines 133 to 150
it('should call user provided `onDraw` after drawing', async function () {
// 1a. Force currentTime to 0
mockTime(0)

// 1b. Layer must be inactive to start
const layer = movie.layers[0]
layer.active = false

// 2a. Prepare options object with onDraw callback
const options = jasmine.createSpyObj('options', ['onDraw'])

// 2b. Play one frame at the beginning of the movie with the spy options
await movie.play(options)

// 3. Make sure onDraw was called
expect(options.onDraw).toHaveBeenCalledTimes(1)
})

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this to the playback section?

Copy link
Contributor Author

@abidjappie abidjappie Mar 4, 2024

Choose a reason for hiding this comment

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

I updated the test to try to use the pattern used in the playback section, but still very basic test. b88be92#diff-f3f0599beaa55e219f66852cc9f533a3c064e0d28d43b550612db7c3987ba0cdR278-R298

src/movie/movie.ts Show resolved Hide resolved
Copy link
Collaborator

@clabe45 clabe45 left a comment

Choose a reason for hiding this comment

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

Thank you so much!!

@clabe45 clabe45 merged commit 134a142 into etro-js:master Mar 5, 2024
3 checks passed
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.

Add onDraw option to Movie.play()
2 participants