⚙ D6139 Add Delete Proxy to Video Sequencer

Eitan Traurig (EitanSomething) created this revision.Oct 28 2019, 1:58 AM

Harbormaster completed remote builds in B5464: Diff 19216.Oct 28 2019, 1:58 AM

Eitan Traurig (EitanSomething) added a project: Video Sequencer.Oct 28 2019, 1:58 AM

Eitan Traurig (EitanSomething) added a subscriber: Video Sequencer.

Clean up return

Harbormaster completed remote builds in B5465: Diff 19217.Oct 28 2019, 2:23 AM

This action can not be undone, so confirmation dialog must be used.
"All proxy files for this strip will be permanently removed. Do you want to continue?"
Or something in that note.

release/scripts/startup/bl_operators/sequencer.py
379–382	It doesn't seem, that this operator requires invoke method, it could be removed.
410–411	Please use a bit more descriptive names
421	also descriptive names
479	I mean it may be correct naming scheme, but it doesn't look right in context of this file. Other editors have this quite mixed, so I don't know. Don't want to nitpick too much, but on the other hand I want to point to potential issues...

This revision now requires changes to proceed.Oct 28 2019, 6:42 AM

Peter Fog (tintwotin) added a subscriber: Peter Fog (tintwotin).Oct 28 2019, 7:00 AM

This comment was removed by Peter Fog (tintwotin).

Peter Fog (tintwotin) added inline comments.Oct 28 2019, 8:54 AM

release/scripts/startup/bl_operators/sequencer.py
479	Maybe it is time to rename these operators to be in consistency with 2.8 class naming? SequencerCrossfadeSounds, SequencerCutMulticam, SequencerDeinterlaceSelectedMovies, SequencerFadesClear, SequencerFadesAdd,

In D6139#141135, @Peter Fog (tintwotin) wrote:

Really great you're having a go at this!

I'm getting an error when trying to delete proxy, when no proxy has been generated for the active Movie Strip, same thing with a sound strip.

record_run_no_gaps.blen_tc is left in the folder, after deleting.

Having more strips selected makes the operator unable to work. The operator should be able to work in two modes: one delete proxies of the active strip and another one delete proxies from all selected strips(the panel suggestion is having different panels for active and for full selection), and in the Sidebar Strip panel +Alt should make the operator work on the full selection.

Maybe a report on how many proxy files has been deleted could be nice in the Statusbar?

record_run_no_gaps.blen_tc being left in the folder is what happens when deleted proxy in the clip editor

Fix bug when multiple strips sellected
Fix bug where record_run_no_gaps.blen_tc and folders aren't deleted
Added Option: Delete Active selected strip proxy

Harbormaster completed remote builds in B5470: Diff 19229.Oct 28 2019, 8:36 PM

Eitan Traurig (EitanSomething) marked 2 inline comments as done.Oct 28 2019, 8:38 PM

More Descriptive Variable names
Fix Crash

Harbormaster completed remote builds in B5472: Diff 19231.Oct 28 2019, 8:54 PM

Eitan Traurig (EitanSomething) added inline comments.Oct 28 2019, 8:54 PM

release/scripts/startup/bl_operators/sequencer.py
479	I can fix the rest of the naming Scheme in another patch

Eitan Traurig (EitanSomething) marked an inline comment as done.Oct 28 2019, 8:55 PM

Digging through WM API, I realized, that the invoke method, was actually used as a confirmation.
It was quite poor in execution, so it would be better if we could explain to user, what is going to happen. Not quite sure if that is possible

In D6139#141348, @Richard Antalik (ISS) wrote:

Digging through WM API, I realized, that the invoke method, was actually used as a confirmation.
It was quite poor in execution, so it would be better if we could explain to user, what is going to happen. Not quite sure if that is possible

I will look into that.
Deleting proxy in the clip editor doesn’t have a confirmation and has the invoke

Code cleanup

Harbormaster completed remote builds in B5478: Diff 19240.Oct 29 2019, 4:09 AM

Campbell Barton (campbellbarton) requested changes to this revision.Oct 29 2019, 2:26 PM

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added inline comments.

release/scripts/startup/bl_operators/sequencer.py
373–380	This doesn't seem necessary or consistent with other operators. Nearly any operator that uses the selection could also use the active-only, however we don't provide this option.
388–391	Poll function should not loop over selection, it's likely to backfire when drawing the interface and slow down interaction. Also, was this tested? it's accessing `self` which isn't defined in classmethods.
412–463	This copy-pastes code from `clip.py`, did you look into sharing logic between these two functions? It seems like this is a case it could be done.

This revision now requires changes to proceed.Oct 29 2019, 2:26 PM

Peter Fog (tintwotin) added inline comments.Oct 29 2019, 3:19 PM

release/scripts/startup/bl_operators/sequencer.py
373–380	The built-in proxy function allows for both building of the active strip and building of proxies for selected strips. Currently there is a Build Proxies function in the menu - so there should also be a delete proxies function. In operators executed from the VSE Menus pretty much all operate on selection, whereas operators executed from the Strip tab in the sidebar are typically only affecting the active strip unless +alt is pressed. If the following reworking of the current proxy panel, which separates the proxy functions into Active in the Strip panel and Selection in the Proxies & Cache tab, should be accepted, then separated delete proxy/proxies will also be necessary: https://developer.blender.org/D6077 So for those reasons, I suggested that Eitan added options for both active and selected strips.

Eitan Traurig (EitanSomething) added inline comments.Oct 29 2019, 6:32 PM

release/scripts/startup/bl_operators/sequencer.py
412–463	I could definitely share code but which file should I put it in

Campbell Barton (campbellbarton) added inline comments.Oct 29 2019, 6:49 PM

release/scripts/startup/bl_operators/sequencer.py
412–463	For the moment you could make a `staticmethod` on the class `CLIP_OT_delete_proxy` in `clip.py`, import and call that. Later we could move to `./release/scripts/modules/bpy_extras/video_utils.py`.

Report proxy deletion information
Add warnings

Harbormaster completed remote builds in B5486: Diff 19256.Oct 29 2019, 6:56 PM

Campbell Barton (campbellbarton) requested changes to this revision.Oct 29 2019, 7:01 PM

Campbell Barton (campbellbarton) added inline comments.

release/scripts/startup/bl_operators/sequencer.py
372–373	These should be set in the classes `execute` function. Also use Python naming convention (lower case w/ underscore separators).

This revision now requires changes to proceed.Oct 29 2019, 7:01 PM

Eitan Traurig (EitanSomething) added inline comments.Oct 30 2019, 3:01 AM

release/scripts/startup/bl_operators/sequencer.py
412–463	I can put it in `./release/scripts/modules/bpy_extras/video_utils.py` right now

Eitan Traurig (EitanSomething) mentioned this in D6171: Python API Move use_proxy_custom_directory from MovieClip to MovieClipProxy.Nov 8 2019, 3:30 PM

Can you look at D6171 it is a python API that would make using the same function much simpler.

Combine sequencer delete proxy and clip delete proxy

Harbormaster completed remote builds in B6133: Diff 20556.Dec 30 2019, 8:53 PM

Rename variables

Harbormaster completed remote builds in B6134: Diff 20557.Dec 30 2019, 8:57 PM

Eitan Traurig (EitanSomething) marked 3 inline comments as done.Dec 30 2019, 8:58 PM

Proxy Custom Directory doesn't work
Proxy Custom File deletes BL_proxy directory, not custom file
Per project setting deletes BL_proxy directory not per-project directory
I haven't tested clip editor, not sure if it is similar there.

Could this operate on selected_sequences rather than active_sequence?

Otherwise looks good.

This revision now requires changes to proceed.Jan 27 2020, 8:43 PM

In D6139#157768, @Richard Antalik (ISS) wrote:

Proxy Custom Directory doesn't work
Proxy Custom File deletes BL_proxy directory, not custom file
Per project setting deletes BL_proxy directory not per-project directory
I haven't tested clip editor, not sure if it is similar there.

Could this operate on selected_sequences rather than active_sequence?

Otherwise looks good.

I found why custom directory isn’t working.
I am using active sequence because that is consistent with the rest of the ui.

In D6139#157778, @Eitan Traurig (EitanSomething) wrote:
I found why custom directory isn’t working.
I am using active sequence because that is consistent with the rest of the ui.

class SEQUENCER_MT_proxy(Menu):
    bl_label = "Proxy"
    def draw(self, _context):
        layout = self.layout

        layout.operator("sequencer.rebuild_proxy")
        layout.operator("sequencer.delete_proxy")

in this menu sequencer.rebuild_proxy will work on selected strips

Found a bunch of other problems with the implementation.I need to add a couple more checks.
I think we need to rework the internals of all of the sequence objects because they are a mess and are inconsistent.