VSE: Snapping feedback
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Jun 30 2021, 5:42 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)
Francesco Siddi (fsiddi)

Commits

rB519c12da4162: VSE: Snapping feedback

Summary

Address initial feedback:

Use Current Frame instead of Playhead
Use checkboxes instead of radio buttons
Hide snapping distance control from UI
Tweak snapping line color - use selected strip color, 50% transparency. Similar to other editors
Draw 2px thick line, since strip outline is also 2px thick

There was suggestion to draw snap line above current frame, but will have to look into if/how this can be done.

Snapping line:

Snapping menu:

Diff Detail

Repository: rB Blender

Event Timeline

Richard Antalik (ISS) requested review of this revision.Jun 30 2021, 5:42 PM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B15520: Diff 38970.Jun 30 2021, 5:42 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jun 30 2021, 5:49 PM

Hans Goudey (HooglyBoogly) added inline comments.Jun 30 2021, 6:13 PM

release/scripts/startup/bl_ui/space_sequencer.py
2289–2291	No need to specify the text here since it's the same as the RNA property names.
source/blender/makesrna/intern/rna_scene.c
3527	Unfortunately defaults in RNA for DNA properties don't do anything currently AFAIK, what does work is using the DNA defaults system.
3528	Did you miss `RNA_def_property_update` for this second property?

Richard Antalik (ISS) marked an inline comment as done.Jun 30 2021, 6:21 PM

Richard Antalik (ISS) added inline comments.

source/blender/makesrna/intern/rna_scene.c
3527	Ah I thought this works for "reset to default" I had some trouble setting up DNA defaults here, but will try again. I thought that I am covered when I initialize these in code and set defaults here...
3528	No, it's in line 3530

Hans Goudey (HooglyBoogly) added inline comments.Jun 30 2021, 6:33 PM

source/blender/makesrna/intern/rna_scene.c
3527	Yeah, I'm not even sure it's supposed to work or not, seems the system is in sort of a broken state.
3528	Oh, sorry!

Address inlines

Harbormaster completed remote builds in B15528: Diff 38987.Jun 30 2021, 11:58 PM

Richard Antalik (ISS) added inline comments.Jul 1 2021, 12:00 AM

source/blender/makesrna/intern/rna_scene.c
3527	I have re-checked DNA defaults and issue was quite obvious - `SequencerToolSettings` is pointer, so it doesn't work. So for now I have removed RNA default values, and I will change `SequencerToolSettings` to be part of `ToolSettings` in another patch.

Hans Goudey (HooglyBoogly) accepted this revision.Jul 1 2021, 12:23 AM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/makesrna/intern/rna_scene.c
3527	You don't have to move it into tool settings, you just need to ensure that it's allocated for the first time with the dna defaults system. But yeah, I agree about doing that separately.

This revision is now accepted and ready to land.Jul 1 2021, 12:23 AM

Hans Goudey (HooglyBoogly) added inline comments.Jul 1 2021, 1:14 AM

source/blender/makesrna/intern/rna_scene.c
3527	Realize I phrased that in a vague way-- you have to make sure to set up the dna defaults system for the `SequencerToolSettings` struct and copy the default data over when the struct is created from scratch.

Richard Antalik (ISS) added inline comments.Jul 1 2021, 1:24 AM

source/blender/makesrna/intern/rna_scene.c
3527	Ah I see, so basically use `DNA_struct_default_alloc` or `DNA_struct_default_get` I would still rather make `SequencerToolSettings` not a pointer, as that simplifies code a bit.

Richard Antalik (ISS) mentioned this in D11745: VSE: Use snapping settings for scrubbing.Jul 1 2021, 10:43 AM

Closed by commit rB519c12da4162: VSE: Snapping feedback (authored by Richard Antalik (ISS)). · Explain WhyJul 1 2021, 10:17 PM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB519c12da4162: VSE: Snapping feedback.

Hadrien Brissaud (hadrien) awarded a token.Jul 2 2021, 12:13 PM

Overall looking good, I only wonder why you chose "Use Current Frame instead of Playhead".

Since it can be quite important what strip the transformed strip is snapping to and strip ends can be close but not the same, it could be made more clear where the transformed strip is snapping to by only drawing the line between the strips in question.

Only the active strip have a white outline, so maybe this color could be used(2 px):

The box select look(1 px, nb. no outline):

Btw. it is quite common to use J or L cuts where the audio is longer than the video, which will make strip ends be very close, but not the same:

In D11759#305259, @Francesco Siddi (fsiddi) wrote:

Overall looking good, I only wonder why you chose "Use Current Frame instead of Playhead".

"Current Frame" is the standard term for this in the UI used everywhere, we decided to avoid using the term "Playhead".

In D11759#306049, @Hans Goudey (HooglyBoogly) wrote:

In D11759#305259, @Francesco Siddi (fsiddi) wrote:

Overall looking good, I only wonder why you chose "Use Current Frame instead of Playhead".

"Current Frame" is the standard term for this in the UI used everywhere, we decided to avoid using the term "Playhead".

I know it's not your choice @Hans Goudey (HooglyBoogly), but when you compare "Current Frame" with playhead, you realize that this choice isn't making Blender more user friendly, professional or industry standard. In other words is "Current Frame" the value and "Playhead" is how it is exposed in the UI(but not in Blender). And it makes the menus incomprehensible: