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

Make effects be callable classes #1096

Closed
mgaitan opened this issue Mar 6, 2020 · 2 comments
Closed

Make effects be callable classes #1096

mgaitan opened this issue Mar 6, 2020 · 2 comments
Labels
feature-request Request for a new feature or additional functionality.

Comments

@mgaitan
Copy link
Collaborator

mgaitan commented Mar 6, 2020

This is just a raw proposal

currently, effects are functions that receive the clip as the first argument, and the rest are arguments for the fx itself. If we refactor them as classes where the init receive the arguments of the fx and __call__ receives the clip instances, the fx function

        >>> clip.fx( volumex, 0.5).fx( resize, 0.3).fx( mirrorx )

could be something like

 >>> clip.fx(volumex(0.5), resize(0.3), mirrorx())

wich, in my opinion is more explicit and readable, and allow better code reuse.

In addition, making them instances allow us to implement further niceties like pipe effects (see #1076 )

clip | volumex(0.5) | resize(0.3) | mirrorx()

Lastly, we could do it just defining a decorator that convert functions to classes dynamically, and adding some type checking to fx to make it backward compatible.

@tburrows13 tburrows13 added the feature-request Request for a new feature or additional functionality. label Mar 6, 2020
@tburrows13
Copy link
Collaborator

This would definitely be an improvement than the current syntax that you demonstrated, but I think that the method that editor.py uses is even better, which is just clip = clip.volumex(0.5).resize(0.3).mirrorx().

However, not all effects and transitions are currently available for use this way because they are not added as methods to VideoClip in editor.py. I propose just updating this list in preference to making them classes like you suggest. (I'll start working on this now).

Feel free to disagree!

@tburrows13
Copy link
Collaborator

My comment is now implemented in #1104, so if you're happy with that, then we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a new feature or additional functionality.
Projects
None yet
Development

No branches or pull requests

2 participants