BLF: Utility to Wrap a String into Multiple Lines #118436

Harley Acheson · 2024-02-18T20:53:07+01:00

Harley Acheson commented

2024-02-18 20:53:07 +01:00

Given a string, font id, and width, return a string vector with the string
wrapped to that width.

We currently only have string wrapping that occurs at the time of text output. But it can be useful to know where a string would break without printing it. For wrapping of uiBut labels, multi-line text input, etc.

Given a string, font id, and width, return a string vector with the string wrapped to that width. --- We currently only have string wrapping that occurs at the time of text output. But it can be useful to know where a string would break without printing it. For wrapping of uiBut labels, multi-line text input, etc.

Harley Acheson added 1 commit 2024-02-18 20:53:18 +01:00

7538763a23 UI: Generic String Wrapping

Given a string, font id, and width, return a string vector with the string
wrapped to that width.

---

We currently only have string wrapping that occurs at the time of text output. But it can be useful to know where a string would break without printing it. For wrapping of uiBut labels, multi-line text input, etc.

Harley Acheson commented

2024-02-18 20:55:00 +01:00

@HooglyBoogly

Does this approach seem reasonable of taking a std::string and outputting a std::vectorstd::string ?

@HooglyBoogly Does this approach seem reasonable of taking a std::string and outputting a std::vector<std::string> ?

Harley Acheson added this to the User Interface project 2024-02-18 20:55:11 +01:00

Hans Goudey requested changes 2024-02-19 01:50:55 +01:00

Hans Goudey left a comment

Having a utility like this seems nice. I have a few comments though:

It should be clear what units the width is in, with variable names or comments (unless maybe this is just documented for the whole API?)
Vector should be used instead of std::vector because of its inline buffer and methods that work well with other Blender data structures
Every std::string requires heap allocation, copying, etc. (unless the string is very short), so I would base the API around StringRef instead. StringRef can also reference a slice of another string for free, since it doesn't need a null terminator.
font->wrap_width = width seems a bit sketchy. Ideally this should be done without changing the font at all, so the font could be const, and the function could be threadsafe.

The commit should probably be titled BLF: Add utility to wrap string into lines or something since this doesn't actually change user-visible behavior.

Having a utility like this seems nice. I have a few comments though: - It should be clear what units the width is in, with variable names or comments (unless maybe this is just documented for the whole API?) - `Vector` should be used instead of `std::vector` because of its inline buffer and methods that work well with other Blender data structures - Every `std::string` requires heap allocation, copying, etc. (unless the string is very short), so I would base the API around `StringRef` instead. `StringRef` can also reference a slice of another string for free, since it doesn't need a null terminator. - `font->wrap_width = width` seems a bit sketchy. Ideally this should be done without changing the font at all, so the font could be const, and the function could be threadsafe. The commit should probably be titled `BLF: Add utility to wrap string into lines` or something since this doesn't actually change user-visible behavior.

source/blender/blenfont/intern/blf.cc

						
				@ -938,0 +939,4 @@

				std::vector<std::string> BLF_string_wrap(int fontid, const std::string str, const int width)

				{

				  FontBLF *font = blf_get(fontid);

				  if (font) {

Hans Goudey commented

2024-02-19 01:43:15 +01:00

Flip the check and return {} inside. It's nice to consistently put error handling at the start of the function

Flip the check and `return {}` inside. It's nice to consistently put error handling at the start of the function

Harley marked this conversation as resolved

source/blender/blenfont/intern/blf_font.cc

						
				@ -1247,0 +1255,4 @@

				{

				  std::vector<std::string> *list = static_cast<std::vector<std::string> *>(str_list_ptr);

				  std::string line(str, str + str_len);

				  list->push_back(line);

Hans Goudey commented

2024-02-19 01:47:16 +01:00

Just as an example, these two lines do a fair amount of expensive things:

First is the line variable. This has to allocate space for str_len and copy the characters
Next is an empty string as the vector increases in size. This is cheap, but worth keeping in mind (
Finally, putting the sliced string into the vector. Copy assignment allocates new characters for the string in the vector, then copies from line. With std::vector, emplace_back is used to construct the new value in place rather than default constructing it then filling its value with copy assignment. C++ also has std::move for this reason: line is just temporary data, we can move it into the vector to avoid the copy

Just as an example, these two lines do a fair amount of expensive things: - First is the `line` variable. This has to allocate space for `str_len` and copy the characters - Next is an empty string as the vector increases in size. This is cheap, but worth keeping in mind ( - Finally, putting the sliced string into the vector. Copy assignment allocates new characters for the string in the vector, then copies from `line`. With `std::vector`, `emplace_back` is used to construct the new value in place rather than default constructing it then filling its value with copy assignment. C++ also has `std::move` for this reason: `line` is just temporary data, we can move it into the vector to avoid the copy

Harley marked this conversation as resolved

source/blender/blenfont/intern/blf_internal.hh Outdated

						
				@ -98,1 +101,4 @@

				std::vector<std::string> blf_font_string_wrap(FontBLF *font,

				                                              const std::string str,

				                                              const int width);

Hans Goudey commented

2024-02-19 01:37:26 +01:00

const is meaningless in declarations for arguments passed by value

`const` is meaningless in declarations for arguments passed by value

Harley marked this conversation as resolved

Harley Acheson added 2 commits 2024-02-19 19:49:24 +01:00

a4be7d2c12 Merge branch 'main' of projects.blender.org:blender/blender into WrapString

e6f910b9ca blender::StringRef and blender::Vector

Harley Acheson changed title from ~~UI: Generic String Wrapping~~ to BLF: Utility to Wrap a String into Multiple Lines

2024-02-19 19:50:07 +01:00

Harley Acheson commented

2024-02-19 19:58:49 +01:00

@HooglyBoogly

It should be clear what units the width is in...

Good point, used max_pixel_width for this

Vector should be used instead of std::vector because...

Yes done and makes sense. Sorry that you have mentioned that to me before.

font->wrap_width = width seems a bit sketchy...

It did seem very sketchy indeed, and gave me the heebie-jeebies while writing it. But yes, makes more sense that the wrap width get passed to blf_font_wrap_apply, using font->wrap_width for the other uses of it.

The commit should probably be titled BLF: Add utility to wrap string into lines or something since this doesn't actually change user-visible behavior.

You are so right. Just a (bad) habit of sticking "UI:" on almost everything.

@HooglyBoogly > - It should be clear what units the width is in... Good point, used `max_pixel_width` for this > - `Vector` should be used instead of `std::vector` because... Yes done and makes sense. Sorry that you have mentioned that to me before. > `font->wrap_width = width` seems a bit sketchy... It did seem very sketchy indeed, and gave me the heebie-jeebies while writing it. But yes, makes more sense that the wrap width get passed to `blf_font_wrap_apply`, using font->wrap_width for the other uses of it. > The commit should probably be titled `BLF: Add utility to wrap string into lines` or something since this doesn't actually change user-visible behavior. You are so right. Just a (bad) habit of sticking "UI:" on almost everything.

👍 1

Hans Goudey requested changes 2024-02-20 03:21:16 +01:00

source/blender/blenfont/intern/blf_font.cc Outdated

						
				@ -1245,0 +1254,4 @@

				                                    GlyphCacheBLF *gc,

				                                    const char *str,

				                                    const size_t str_len,

				                                    ft_pix pen_y,

Hans Goudey commented

2024-02-20 03:20:49 +01:00

Unused variable pen_y? and gc?

Unused variable `pen_y`? and `gc`?

Harley marked this conversation as resolved

source/blender/blenfont/intern/blf_font.cc Outdated

						
				@ -1245,0 +1262,4 @@

				  list->append(line);

				}

				blender::Vector<std::string> blf_font_string_wrap(FontBLF *font,

Hans Goudey commented

2024-02-20 03:20:21 +01:00

To be clear, I meant this should return Vector<StringRef>. Then this function doesn't need to do any allocation or copying at all. The only requirement is that the lifetime of the input string is at least as long as the lifetime of the output. But that's a nice tradeoff.

To be clear, I meant this should return `Vector<StringRef>`. Then this function doesn't need to do any allocation or copying at all. The only requirement is that the lifetime of the input string is at least as long as the lifetime of the output. But that's a nice tradeoff.

Harley marked this conversation as resolved

source/blender/blenfont/intern/blf_internal.hh Outdated

						
				@ -98,1 +103,4 @@

				blender::Vector<std::string> blf_font_string_wrap(FontBLF *font,

				                                                  blender::StringRef str,

				                                                  const int max_pixel_width);

Hans Goudey commented

2024-02-20 03:20:18 +01:00

const int means the same thing as int here in the declaration, so typically const isn't included here.

`const int` means the same thing as `int` here in the declaration, so typically `const` isn't included here.

Harley marked this conversation as resolved

Harley Acheson added 2 commits 2024-02-20 16:36:25 +01:00

402fa5229f Merge branch 'main' of projects.blender.org:blender/blender into WrapString

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

f352a27829 Using blender::StringRef

Harley Acheson commented

2024-02-20 16:40:45 +01:00

@HooglyBoogly

Thanks for your patience. I really didn't appreciate what StringRef brings to this.

For the simplest case where the string is within the limits, there is barely a difference between just measuring it and calling this wrapping function and examining the vector size, and that's pretty cool.

@HooglyBoogly Thanks for your patience. I really didn't appreciate what StringRef brings to this. For the simplest case where the string is within the limits, there is barely a difference between just measuring it and calling this wrapping function and examining the vector size, and that's pretty cool.

Hans Goudey approved these changes 2024-02-20 16:45:11 +01:00

Hans Goudey left a comment

Looking good now, thanks for the iterations!

source/blender/blenfont/BLF_api.hh Outdated

						
				@ -8,8 +8,12 @@

				#pragma once

				#include <string>

Hans Goudey commented

2024-02-20 16:44:39 +01:00

Unnecessary include now :)

Harley marked this conversation as resolved

source/blender/blenfont/intern/blf_internal.hh Outdated

						
				@ -8,6 +8,11 @@

				#pragma once

				#include <string>

Hans Goudey commented

2024-02-20 16:44:55 +01:00

And here too

Harley marked this conversation as resolved

Hans Goudey commented

2024-02-20 16:45:25 +01:00

@blender-bot build

Harley Acheson added 1 commit 2024-02-20 16:54:48 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

f77ac43065 Unnecessary includes

Harley Acheson commented

2024-02-20 17:47:34 +01:00

@blender-bot build

Harley Acheson added 1 commit 2024-02-20 20:55:55 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

202659e9d3 unused argument, assignment type warning

Harley Acheson commented

2024-02-20 20:56:35 +01:00

@blender-bot build

Harley Acheson merged commit af4b09a61c into main

2024-02-21 01:19:14 +01:00

Harley Acheson referenced this issue from a commit

2024-02-21 01:19:14 +01:00

BLF: Utility to Wrap a String into Multiple Lines

Harley Acheson deleted branch WrapString

2024-02-21 01:19:16 +01:00

Harley Acheson referenced this pull request

2024-02-21 20:38:57 +01:00

WIP: UI: Wrap Buttons #118579

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

BLF: Utility to Wrap a String into Multiple Lines #118436