VSE: Add text alignment feature #126660
No reviewers
Aras Pranckevicius
Pablo Vazquez
Francesco Siddi
Sergey Sharybin
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
8 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#126660
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "iss/blender:text-edit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Previously, alignment did exist, but it only changed whole text block
position in relation to a fixed point. This was later renamed to "Anchor".
Now it correctly aligns each line of text. Alignment works with newline
character and word wrapping.
Currently newline characters can't be entered directly, but this should
be resolved soon.
To keep existing anchoring feature, new DNA fields are added and
values from old alignment are copied there.
This PR is part of bigger change [1], and originally I expected to
implement this feature at later stage. But the design called for drawing
text character by character, which would mean, that I would have to
rewrite text alignment anyway.
To render the text, a struct is built, where position and width of each
character is stored. In addition, width of each line is stored. This allows
to implement proper text alignment feature, instead of existing
anchoring. Text is then drawn character by character in a loop.
Since text drawing is simplified, text box height is now defined by line
height. previously it changed height based on tallest character.
[1] #126547
WIP: VSE: Draw text effect character by characterto VSE: Add text alignment feature@Harley I think, that BLF is in your ballpark, so thought I will poke you. Did not change anything there, just took some responsibility on VSE side. If you have any thoughts, feel free to share.
@ -92,2 +97,4 @@void SEQ_effect_text_font_unload(TextVars *data, bool do_id_user);void SEQ_effect_text_font_load(TextVars *data, bool do_id_user);using namespace blender;Minor: I'd think that
using namespace blenderis not needed since what follows is already a sub-namesspace of that (blender::seq)@iss
Hey Richard. I am only checking this out now, so don't read any of the following with any negativity at all. The following are just random thoughts about this not necessarily about this PR in particular.
I'd first have to give my sympathies for trying to follow any of the vfont stuff. That code is just a terrible pile of hacks upon hacks and I don't think I'd recommend anyone copying anything from it. It does do some cool things though, like text on a curve, selection, etc but yikes.
Looking at the capture above I would guess that the spacing might be incorrect. Probably because BLF isn't exposing the glyph "advance" you need. Using BLF_width of the character would often be wider than the amount to move forward along the baseline. BLF_width works reasonably for a string but I'm not sure it is good enough to use for per-character placement. The min of that bounding box is left side of the character's bounding box, which can be negative. And the max of it is the greater of the advance and character width. I think you would just need to get the glyph's advance_x, which is how much to move right after a character is placed. It is in 64th of pixel. It isn't publicly exposed now, but would be easy to do.
I think I see a desire to do all this character-by-character, probably so that you can do per-character effects like bold and italics, different weights etc per character. But that might be a difficult approach. Character-by-character can be difficult because there are many times when there isn't a one-to-one relationship between characters in the string and the glyphs that are shown.
Ideally you want to separate the input string from the output glyphs, allowing for the possibility that the two might differ in number. Sometimes a character entered might turn into multiple glyphs. And sometimes multiple characters turn into a single glyph, like with ligatures.
One idea is to just not do this character by character but by runs of text. For each character in your input string you also keep data for font, boldness, obliqueness, weight, whatever. Then when printing you just do multiple characters where all is the same, so generally sending a string to BLF and have it write to bitmaps for you. Then the spacing issue wouldn't matter much as that would only happen between runs.
Or if you don't want to use BLF_draw_buffer for the output you could assemble these runs, send them to some new BLF function that returns a bunch of arrays, one containing all the glyphs that represent that string, plus others containing all the positioning information. This kind of approach would work for all accented characters, ligatures, complex language, and even right-to-left languages (although these are hard to edit).
But again, don't take any of this as negativity. I just don't want you to get frustrated with this.
I do have some (hopefully) future BLF code that might be useful to you for this. Although made for complex and RTL languages, it is code that necessarily takes whole strings and gives back arrays of glyphs and positioning information. I'll try to update that ASAP.
@Harley Thanks for your input. Apart from justification and word wrapping, the more important aspect is the actual text editing, as user has to position the cursor. I have tested only limited subset of unicode, and only from languages I am familiar with, so it would be nice to have some examples where this breaks. I guess, that arabic would be one that is problematic, but not sure if there are others. There would be probably examples on google.
Having BLF function to give array of glyphs with positions would be very good way to do this indeed. This way word wrapping and alignment can be kept in BLF module.
I would say, that this PR at least shows me, that it is possible to do this in quite readable and relatively simple form (disregarding sudden change of direction of text drawing). I still have ideas to improve this a bit. I agree, that the VFont code is pretty wild.
In any case, I will check limitations as mentioned, and depending on that I would discuss further whether we would want to go ahead with this implementation until the limitations can be resolved. Last thing I want is to introduce regressions.
As @Harley says, the glyph advancing right now is not correct. This can be easily seen in cursive/script fonts, e.g. "Dancing Script" (https://fontlibrary.org/en/font/dancing). Current main branch looks like this:
Whereas in this PR it looks like this:
Additionally, I'm not entirely sure if controlling both "anchor" and "alignment" with the same setting makes a lot of sense. It feels a bit weird that if I just want to change how multi-line text is aligned, changing horizontal alignment from "left" to "center" moves the whole text block by a lot. My opinion is that maybe "alignment" and "anchor" should be separate controls, or, perhaps, the "anchor" is not really needed at all?
My gut feeling is that to use this approach would require that
BLF_draw_bufferoptionally return information about what it drew. Probably the typographical "top" in this case, which would be the distance from the top of the bitmap to the text baseline. A "left" value for the left bearing of the leftmost glyph, and the "advance" of the last glyph. I think that would be enough that you could join multiple results into single strings that line up properly, height to do so vertically and then matching the last advance value to the next left value.Yes, anchor was dropped with this PR. Alignment should work in same way as 3D viewport. I have tested this only with builtin font so far and focused more on handling unicode correctly. I see, that there is bunch of work to do here, so will set this as WIP again.
VSE: Add text alignment featureto WIP: VSE: Add text alignment featureJust realized, that I had changes in naming Anchor -> Alignment uncommitted, so pushed these just now.
To Harley's point about combining sequences etc., to me it seems like this branch, besides "wrong" glyph advancement, does not make things worse for VSE text strips. Some combining sequences were already not handled "properly", like the famous Zalgo text or "strike through effect" that is achieved by combining sequences was not already rendered correctly. Here's how it looks like on main:
And on this branch:
-- there are character advance distance issues, but otherwise both "zalgo" and "strike through" cases are "incorrect" on main too. Attached the blend file with this.
My thinking was that maybe anchor should not exist at all functionality wise. Like today, center alignment works as I would expect, all good:
But intuitively, if I pick "left aligned", I would expect the overall box to stay in the same place (i.e. bounding box is still "centered"), just the text itself to get left aligned within the box. But now it both aligns the text, and moves the box to be elsewhere on screen:
(of course, with my suggestion/idea, it would be a good question what "vertical alignment" would even do -- perhaps nothing, and it should be removed)
Maybe the anchor function should just work like the pivot point of images, but for the text box? And not affect alignment of text inside the text box at all.
@blender-bot package
Package build started. Download here when ready.
Part of the confusion between Anchor and Alignment is properly that the text box is not visible. The "text box" could be drawn in the Preview as ex. dotted lines for the Wrap width + top + bottom, so it would be clear what is inside and outside, especially if the user starts transforming on the text strip. Ex. if moved up, some of the text will disappear in the bottom.
Having the Text Box visible will make it clear that Alignment will place and align the text INSIDE the box, but not move the box. Whereas, the anchor function (could be drawn in Preview too) only has relation to what point of the text box which will snap to the Position X & Y (Strip position).
The Location X & Y (position inside the Text Box) could also be drawn in Preview.
I was checking behavior of 3D viewport without textbox defined. With textbox this behaves more like you propose.
In VSE, textbox is pretty much defined by wrap width, with no control over the height. So this probably could be changed.
Not sure, if having text box centered to image center with left or right alignment is great idea though. I can try it...
I did not mention this in #126547, but it is quite reasonable to expect, that text transformation would be animated. in that case, it is good idea to have it vertically centered to it's pivot point by default. So I would agree, this option could be dropped. In this case, text box height would be dynamic and not controlled by user.
But on the other hand this could be considered as convenience feature, to place text on top or bottom of the screen.
Personally I am inclined to dropping vertical alignment, as goal is to move the editing to the preview anyway. Argument against may be, that with sane defaults, it would be easier to get consistent text placement, but that is easily achievable anyway.
Another way to approach this, is to build the functionality around use cases, some of the typical ones are ex:
(In addition to the visual challenges, there are challenges like pasting in the widget, only pastes up to a line break, and it only can hold 512 characters, and possible enhancements like optional Text-Block (from the Text Editor) selection).
This would work, assuming larger buffer than we have currently. At least just scrolling of plain text.
IMO better would be to resolve #109943, since then you can do more "artsy" credits.
You can do this already and I just broke it in this PR :(
It's technically possible, but not implemented.
By snapping I guess? Not exactly sure what you mean.
This PR only implements alignment feaure.
WIP: VSE: Add text alignment featureto VSE: Add text alignment feature@blender-bot package
Package build started. Download here when ready.
Oh, this is looking really nice. With the addition of your
BLF_glyph_advancethe spacing looks almost perfect. And I think "perfect" might be an easy step.We have an advancing pen positioning. You place a new glyph at that position and then move the pen by that character's advance amount. For so many fonts and uses this is enough.
The last step is that for some uses and some characters in some fonts the "left" edge of a character is not zero, but negative. So while at the current pen position you place the next glyph based on its "left" value. That allows the character to move back a bit if needed. Note that the character's "advance" though is still based on the pen position, and not considering its "left".
You see this a bit with some italic fonts where they have their tops leaning backwards a bit. They might all have normal advances, but most might have a negative left value. Yes, overall this makes no real difference in the spacing but only where that bit of text connects to the non-italic text before or after it. And that doesn't apply to you.
But you do also get the odd weird-looking font where they need to use negative left values for some effects.
There is one important use though that you get for free once you use that left value, and that is "Combining Diacritical Marks". These characters will have negative left values and zero for advances. Although Unicode contains many characters with marks already in them, like "a with grave accent" (U+00E0) you can also make the same with an "a" followed by "Combining Grave Accent" (U+0300). Why? There's piles of these things and they combine with any character at all. This the former "à" while this is the two codepoints comprising the latter "à"
This left value for the glyph can be obtained in the same way as the advance, as g->pos[0]. If doing so you might want to get the glyphs "top" value too, as g->pos[1]. The typographical "top" being a measure from the top of the bitmap down to the baseline.
Ok, will try some of these. Sounds easy to fix this
Ah I see! I thought, that grave accent would be combined with main character to give single unicode character. On that note you mentioned separation of glyphs and input string - technically
CharInfocould be calledGlyphInfo, since it allows byte sequence of arbitrary length. However to implement this well enough, I would have to know, that there is character composition going on, and I suspect, that BLF module does not care about this - it just draws as instructed.That said, it sounds, like there is quite good argument of implementing this on BLF side as you mentioned, since even current UI does not handle 2 code variant of
àwell. I am not sure what shape of data other areas would require though. So I feel like I should not jump onto this jst yet and keep working on this on VSE side.Also on the other hand, many areas don't need to know these kind of details, so my code is definitely not optimized for best performance.
Right, so I add this to the glyph position, and it should work. Sounds easy...
Edit: Famous last words, It does not quite work as expected. Will have to dig a bit more. In any case, I probably wouldn't consider this as show stopper, so in meantime will move on to editing in preview feature.
@blender-bot package
Package build started. Download here when ready.
We don't get glyph composition or anything similar. We can only just ask for a codepoint in the official Unicode list and get that image. Using multiple characters in a row that draw on top of each other with those Combining Diacritical Marks is the closest we get.
Well that is how it works with our current design of only directly using FreeType. We ask for a particular codepoint from a particular font and get back a bitmap of that glyph.
A future step for us will be to add a "text shaper", like Harfbuzz. Despite the name "text shaper", it still doesn't actually alter any glyphs or compose or anything like that. What that does is examine an entire string (or subset) and will give you back a more ideal string of glyphs. At the simplest we give it a string like "flit" and it could give us back only three glyphs, with the first being one that combines "f" and "l" together. But it doesn't actually combine them but just allow using a special ligature glyph that the font designer has created for that purpose. Interesting is this kind of substitution can even be done with glyphs that are in the font that don't actually have a place in the Unicode list. So as the font designer you could create a glyph that looks like an arrow and then add to the the font's substitution (gsub) table that if you see "-" and ">" together to use that instead.
It is this kind of complex substitution that requires a "text shaper". Their most useful purpose is for languages like Arabic. That language is like our cursive handwriting. A letter used at the beginning of a word will look different than if it is in the middle or end. So Harfbuzz gives you a different list of glyphs back that are quite unrelated to the ones you give it.
For us this change would mean we no longer can do things character by character but have to deal with portions of strings instead. We give that portion (same language, same direction, same boldness, color, etc) and get back a list of glyphs and positioning information. This will also include combining diacritical marks too. Interestingly it not only gives us back that left, top, and advance but also secondary x and y nudge values....
With those accent characters they just have a single "left" value in the font. But some of these need to be in specific places over the letter, like centered. Since the letters vary in width Harfbuzz uses this extra nudges to put those in ideal places.
@blender-bot package
Package build started. Download here when ready.
Users are properly going to wonder why it looks like nothing happens, when changing the alignment on the default text:
Comparing to the 3D View, where text position is changed:
Just for completeness, there is one (more) obscure and annoying thing to mention.
This would allow users to select a monospaced font. You can tell they have done so because the font will have BLF_MONOSPACED flag. I'd assume everything works fine, although technically best to use the spacing from
BLF_fixed_width, but that might not matter for you here since VSE won't have any need to force a variable-width font to behave as fixed-width.But while processing monospaced characters it is best to send each (char32_t) character through
BLI_wcwidth_safe(or related), which returns a column count for that character. Basically just multiply that by the spacing width. This just allows some monospaced characters to display double-wide. Yes, this is an old and weird standard. And no, they won't have double-width advance values because many systems won't flag a font as monospaced unless every character has the same advance. And yet some are meant too be double. So weird.Please don't ignore failing tests on the CI/CD, and also run them locally. And also don't assume that failing tests are fine because logic is changing anyway: verify that the tests are failing in the expected manner.
Using tools like
UndefinedBehaviorSanitizerform LLVM is also something heavily recommended to catch issues early on, without having too much overhead on the run time (compared to more sophisticated tools like valgrind).The immediate issue with the code is that the wrapping code relies on an uninitialized variable
character.do_wrapinsequencer/intern/effects.cc:3239. The naive solution to this could beWith this fix the
text outline stacktest is failing in the expected manner: the wrapper parts of the text have center alignment, instead of left.The box drawing in the
text optionsandtext shadow outlineis failing unexpectedly: why does fix to horizontal alignment has anything to do with the vertical size of text box?Wrapping of words without spaces seems to behave differently now (they don't wrap anymore?).
The PR is removing vertical alignment option, but mentions nothing about it and motivation behind it. I don't know why we need to remove vertical alignment to fix horizontal one.
For the caching I also wouldn't mind having a stronger motivation behind it. If it is something very complex to compute, sure, we might need to add cache at some point. But is probably not something that should be the first focus for initial feature implementation.
Going cache direction adds the complexity of ownership, invalidation, etc.
In practice this currently leads to situation when the cache does not react correctly when text has animated wrapping or size (and possibly many other properties as well). Those could be fixed with some cache invalidation added somewhere, but it does not intuitively feel the cleanest solution. What is the relative time of re-calculating wrapping and doing an actual rendering?
P.S. Please be more mindful to the reviewer's time, and do as much of testing and as much of clear presentation as possible on your end.
I did not look at the tests yet. I was working on functionality, so I can get feedback for #127239 as early as possible. It's strange, that this did not fail for me so far during testing, so I incorrectly assumed, that initializing this struct would clear the memory as with
MEM_callocN. I should have looked at implementation.This was rather experimental change from earlier discussion with Aras. It is briefly mentioned at the end of PR description - Idea is, that instead of using vertical alignment, you just move text where you want it by mouse. It's quite unlikely that you want text to be at absolute top or bottom anyway. I can move this change to different PR or should I clarify this bit more?.
I did not measure the absolute time to compute this data, but it does not visibly affect the performance. Originally I thought, that I would have function to translate this cache back to
TextVarsdata, but that is not what I ended up doing in #127239. So this cache does not need to be pernament as it is now..I am doing what I can. Perhaps I should have clarified in PR description what kind of feedback I am looking for at this point (overall design and functionality in this case). I may still need to make changes as I work on text editing part anyway.
@Sergey I changed the PR, so that runtime data is recalculated for each frame, but this complicates further development a bit:
SeqRenderData. Either I make it not to rely on that, so I will have to add a bunch of boilerplate code to bunch of places and drag needed references along, or I would have to re-create this huge struct and use it in places it is not really intended to be used at. In any case this is also quite error prone, since it is easy for some minor change to be done in one place and not the other.So I would suggest to keep
TextVarsRuntimeas runtime in DNA, it would be cleared before rendering each frame, so it would not have problems with invalidation, but UI will be able to use this as well. I would have to check how this behaves with Frame Overlay feature and during rendering, but I am pretty sure these cases can be dealt with rather easily.Any opinions on this?
consts 50a40fe9a6@blender-bot package
Package build started. Download here when ready.
Reviewed the functionality with @pablovazquez and @Hjalti - it's not clear why the anchor functionality has been removed. That functionality is useful for accurately positioning the text block.
We suggest to simply add the text alignment behavior as part of the Style subpanel and keep the other settings as before.
@fsiddi @pablovazquez
As this version of Alignment doesn't have any effect on single line texts, maybe it should be renamed "Multiline Alignment", qua: #126660 (comment)
And since Wrap Width is crucial for adjusting multiline length, this should properly be next to Alignment. There could even be a sub panel called Multiline containing: Wrap Width and Adjustment.
BTW. on the general Text strip UI, there was a request for using more sub-panels:
7e5040783c/space_sequencer.pyStyle refers to font style, so I will add this to (text) layout panel.
@tintwotin I don't see point of doing changes like that since there is further development and this PR is purely strategic.
Just a note, that I will look bit more into why box size is different in font in our test files. Fonts I have been testing with have nearly identical size to builds from main branch. Could be one of suggestions by Harley.
@iss I've tested the current state of the branch, it works the way I'd expect it to.
We're only missing text box and editing text in preview, and a multi-line UI editing element. But all that is for a separate PR :)
For this one
sequencer_render_effectsis failing, but seems to be due to box size, and you've mentioned you're looking into it. Once that is figured out, I don't really see stoppers for this PR.BLF_boundbox()on per-line basis. 2b25e91eb0There is some weird 1px offset in some test files. Just checked it quickly, and it looks like either rounding error when image is centered or the way I calculate line height. Seems fixable in either case, but I expected some imperfections from the start.
@blender-bot build
@ -3085,0 +3107,4 @@int byte_offset = 0;while (byte_offset <= BLI_strnlen(data->text, sizeof(data->text))) {const char *str = data->text + byte_offset;const int char_length = BLI_str_utf8_size_or_error(str);BLI_str_utf8_size_or_error can return -1 for malformed UTF8 sequences, in which case the whole loop would probably start walking into nonsensical memory and crash. Maybe break out of the loop of char_length is <= 0?
I tried to avoid error handling where possible with UTF8 stuff, probably autocompleted this one by mistake. Thanks for catching this.
@ -3440,2 +3446,3 @@RNA_def_property_enum_items(prop, text_anchor_x_items);RNA_def_property_ui_text(prop, "Align Y", "Align the text along the Y axis, relative to the text bounds");prop, "Anchor X", "Position of the text box relative to image center in X axis");Is the tooltip technically correct? I think the position of the text box is not relative to the image center, but rather to the "Location" property (which defaults to image center, yes).
Maybe wording of this one (and same for anchor_y) should be like "Horizontal position of the text box relative to Location"
Sounds good.
@ -3433,2 +3440,2 @@RNA_def_property_ui_text(prop, "Align X", "Align the text along the X axis, relative to the text bounds");RNA_def_property_enum_items(prop, text_alignment_x_items);RNA_def_property_ui_text(prop, "Align X", "Align the text along the X axis");Maybe "Horizontal alignment for multi-line text" sound easier to understand than "Align the text along the X axis"
Proposed one sounds bit weird. To me it sounds like reference to non-existent feature. I would phrase it as "Alignment of lines when text is wrapped" which still sounds bit weird.
Honestly I am not sure if this really needs further clarification. Especially if you consider editing in preview.
Fair! How about just "Horizontal text alignment"?
Would "Horizontal line alignment" address that this works with multiple lines only and is still understandable enough?
"Text alignment" still sounds better to me.
"Text Alignment" sounds good to me too
@ -4,6 +4,11 @@#pragma once#include "BLI_index_range.hh"Why BLI_index_range.hh and BLI_span.hh includes are needed in this header?
Eeh forgot to check added includes.
@ -92,0 +110,4 @@int width;};struct TextVarsRuntime {All of the TextVarsRuntime, LineInfo, CharInfo seem to be only used within
effects.cc, so they don't need to be in any header. And maybe rename TextVarsRuntime to some other name, since usually "Runtime" structs are for runtime fields within DNA types. Which is not the case here.I'd go with, I dunno, TextDrawCharInfo, TextDrawLineInfo, TextDrawInfo or something similar.
Correct, but this PR is pretty much just cut off from #127239 and only submitted separately for making review process bit simpler.
Runtime struct will be put into DNA again, It will just be written over on each redraw.
Ah ok then!
@ -13,2 +13,4 @@#include <cstring>#include "BLI_vector.hh"#include "DNA_curve_types.h"Why is DNA_curve_types.h include needed here?
Honestly not sure why I ever needed it in this development...
@ -3085,0 +3105,4 @@{blender::Vector<CharInfo> characters;int byte_offset = 0;while (byte_offset <= BLI_strnlen(data->text, sizeof(data->text))) {No need to keep on calling BLI_strnlen on each loop iteration, call it once before the loop
I would assume, that compiler would optimize this out, since
data->textis not modified in the loop and variable is notvolatile. Honestly I just like my functions as short as they can be...The compiler can not know/assume that, generally. Inside the loop there's a function call to
BLF_glyph_advanceand the compiler has no idea whatsoever what it does -- it can very much end up modifyingdata->textor do anything imaginable.@ -3085,0 +3141,4 @@/* First pass: Find characters where line has to be broken. */for (CharInfo &character : characters) {if (character.str_ptr[0] == ' ') {Should this break only on literal space, or on other "white space" characters (e.g.
\t)?I was thinking about this, so far I have checked how BLF handles these and went with that behavior. It seems to be rendered as space and does not break text.
Tab support will have to be added as it is useful to have them. I would probably do it as separate PR. Ideally support could be added to BLF with minimal changes to sequencer, but that may be unwanted so would have to define behavior in VSE code explicitly.
That said, yes, technically lines could be broken on all whitespace characters. Just now checked for existing functions, there is
text_check_whitespace()and it seems to be limited to ACSII so far. So I could addtext_check_may_break_whitespace()function.@ -3085,0 +3174,4 @@}}static int text_box_width_get(const blender::Vector<LineInfo> lines)No need to copy lines vector into this function, passing by reference would work fine
@ -3085,0 +3262,4 @@}runtime.text_boundbox = line_boxes.first();for (rcti box : line_boxes) {Maybe
for (const rcti &box, no need to copy them for each loop iterationI added a handful of comments that while minor, should perhaps be addressed (the "possible crash on invalid text data" is perhaps the main one, but then some others wrt small performance issues too). Overall very nice!
Just tested it and works great! Glad to see Anchors back. The combination of anchor and alignment gives pretty much all combinations needed.
I'd do some UI tweaks but that can happen as part of the planned refresh to the Text strip panel layout.
@blender-bot package
Package build started. Download here when ready.
@pablovazquez I did not press the issue here, because it is simpler to keep original functionality for this PR, but I would like to not have anchor and text position in current form. We have pivot point and translation properties already, so I would like to combine these. But I would need to understand how these are used in the first place. Perhaps what I want is not possible, but will keep this discussion for later.
Looks good!
Thanks for the PR, update, reviews and tests!
Checkout
From your project repository, check out a new branch and test the changes.