Paths

Table of Contentst

Differential D9468

Add background rectangle option to video sequencer Text strip
ClosedPublic
Actions

Authored by Paul Melis (paulmelis) on Nov 5 2020, 1:32 PM.

Details

Reviewers

Richard Antalik (ISS)

Commits

rB235c309e5f86: Add background rectangle option to video sequencer Text strip

Summary

This adds a Box option to the Text strip's style properties, plus related Box Margin value:

When enabled the text is placed on top of a solid-filled rectangle of a chosen color, as shown below:

When the box option is disabled the text strip works the same as it does now. When the box option is enabled the meaning of the Shadow option changes to provide a drop-shadow on the rectangle (and not on the text itself). The latter made more sense to me.

The box margin is specified as a fraction of the image width. The offset of the drop-down box shadow is fixed to a specific fraction of the image width as well.

I tested this feature on a movie of a couple of minutes containing dozens of text strips (all with box background), edge cases like multi-line strings and text overlapping the image edges.

Diff Detail

Repository: rB Blender

Event Timeline

Paul Melis (paulmelis) requested review of this revision.Nov 5 2020, 1:32 PM

Paul Melis (paulmelis) created this revision.

Hi Richard, I'm adding you to the CC of this patch, hope that's ok.

In D9468#234878, @Paul Melis (paulmelis) wrote:

Hi Richard, I'm adding you to the CC of this patch, hope that's ok.

Thanks for patch! I will check this out soon.

Checking functionality first, I found some issues:

When you enable box on text strip with default settings you can't see the text. I would suggest to set perhaps gray color as default. Perhaps semi-transparent white would be even better.

Another issue is, that some characters are drawn below the line. I don't want to sanitize extremes, but this would be nice if it could be resolved, though not sure if that is possible with current BLF API

For code itself, please refer to style guide - https://wiki.blender.org/wiki/Style_Guide/C_Cpp
Patches should conform to this style in order to be accepted. If you use function I recommend, most style issues will be gone, but it is still worth looking into.

source/blender/sequencer/intern/effects.c
3964–4033	`IMB_rectfill_area_replace()` was added recently, I think you can use it here. Just call it here once for shadow and then for box.

This revision now requires changes to proceed.Nov 5 2020, 5:01 PM

Richard Antalik (ISS) added a project: Video Sequencer.Nov 5 2020, 5:07 PM

Thanks for the feedback! I think I have all the box sizing correct now (phew). Turns out all of the font-specific values were not really needed as using the bbox is enough. Also, using IMB_rectfill_area_replace really simplifies things.

I also checked against the coding style guide, think I have most issues resolved now.

Good job!

Will commit this tomorrow.

This revision is now accepted and ready to land.Nov 5 2020, 11:29 PM

Closed by commit rB235c309e5f86: Add background rectangle option to video sequencer Text strip (authored by Paul Melis (paulmelis), committed by Richard Antalik (ISS)). · Explain WhyNov 6 2020, 4:09 PM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB235c309e5f86: Add background rectangle option to video sequencer Text strip.

Congrats on having this patch committed. I hope it's okay I comment here evenso.

The Box Margin value seems odd. From a user perspective it is not very clear that the fraction value relates to the width, and the steps on ex. 4k material seems too high:

Can a more logic value be thought up?

As this value can be keyframed, I think you should use decorate(add the animation widget on the right hand side) - it'll also look better in the UI.

As this is most typically used for subtitles I think the default color should be black, maybe with a bit of alpha.

In D9468#235548, @Peter Fog (tintwotin) wrote:

Congrats on having this patch committed. I hope it's okay I comment here evenso.

The Box Margin value seems odd. From a user perspective it is not very clear that the fraction value relates to the width,

Well, it's consistent with other values on the text strip also being based on the image width (or height), like Location X and Wrap Width. It would be confusing to make this value work differently.

and the steps on ex. 4k material seems too high:

Is there a way to set the step size on the property to a lower value?

Can a more logic value be thought up?

If anything, the border margin value should result in the same size in pixels independent of the width of the text string or the current proxy setting. That leaves pretty much only a value based on image width (or height).

As this value can be keyframed, I think you should use decorate(add the animation widget on the right hand side) - it'll also look better in the UI.

Okay, that can be added

As this is most typically used for subtitles I think the default color should be black, maybe with a bit of alpha.

I can make it consistent with the default shadow color

You properly shouldn't do anything until @Richard Antalik (ISS) reflects on those suggestions. I shared a few more thoughts on the UI here: https://blender.chat/channel/vse?msg=YrRK6n3WgmyYQZMgW

On the values, there is this patch waiting for someone to figure out the versioning code, which changes the Location values to %: https://developer.blender.org/D7768

@Peter Fog (tintwotin) Thanks for suggestions, I was thinking about margin size as well. It could be in pixels, that may be more sensible. When I tested implementation, the range was as expected.
Pixels can get you better precision, but fraction "feels" more consistent if you work with different resolutions. So it's prefference issue really.

Also thanks for catching missing decorator, will add this.

My post on the chat(the two bounding box color options should also be in the same column):

I saw that, but I think I had some issues with that proposal. And I am not sure if it would make sense to put this option under that category. I had no time to respond during weekend, sorry.