Page MenuHome

Geometry Nodes: String to Curves
ClosedPublic

Authored by Erik Abrahamsson (erik85) on Jun 7 2021, 1:32 AM.
Tokens
"Mountain of Wealth" token, awarded by duarteframos."Love" token, awarded by kenziemac130."Love" token, awarded by damian."Like" token, awarded by Fracture128."Love" token, awarded by digim0nk."Love" token, awarded by Schamph."Love" token, awarded by EAW."Love" token, awarded by cmzw."Love" token, awarded by kursadk."Love" token, awarded by Roger_Menzi."Love" token, awarded by julperado."Like" token, awarded by IPv6."Love" token, awarded by lopoIsaac."Love" token, awarded by Bit."Love" token, awarded by jimman2003."Love" token, awarded by mindinsomnia."Love" token, awarded by aiekick."Like" token, awarded by GeorgiaPacific."Love" token, awarded by Draise."Love" token, awarded by pecador."Love" token, awarded by AndyCuccaro."Love" token, awarded by RC12."Love" token, awarded by Rusculleda."Love" token, awarded by someuser."Love" token, awarded by HEYPictures."Burninate" token, awarded by ugosantana.

Details

Summary

This patch adds a node that generates a text paragraph as curve instances.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Some thoughts:

  • This is great!
  • In "Truncate" mode it would be really cool to have a string output of all the text that didn't fit into the box
  • The "Rotation" input seems to make the other inputs not work
  • The fields can have PROP_TRANSLATION, PROP_EULER, and PROP_XYZ
  • "Char Spacing" should be "Character Spacing"
  • The node could be a bit wider by default, it looks very vertical right now
  • Maybe "Charcode Offset" is useful in some cases, but I don't think we should expose it like that. I think it's better to separate the intricacies of string processing into separate nodes or separate design discussions.
  • Proper description in the RNA properties would be helpful
  • The node could output anonymous attributes on the spline domain for character index, word index, and line index for further processing afterwards
  • I guess the internal pointcloud is to make the field evaluation work? Hopefully that won't be necessary in the proper fields implementation ;)

Some thoughts:

  • In "Truncate" mode it would be really cool to have a string output of all the text that didn't fit into the box

Great idea.

  • The "Rotation" input seems to make the other inputs not work

Yeah, something is wrong with that.

  • Maybe "Charcode Offset" is useful in some cases, but I don't think we should expose it like that. I think it's better to separate the intricacies of string processing into separate nodes or separate design discussions.

Yes that makes sense.

Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)

Added some comments and fixed bugs involving fields.

Are you using a debug build? I had some assert fail when changing the min width, in truncate mode I think.
BLI_assert failed: source/blender/blenkernel/intern/font.c:970, vfont_to_curve(), at 'compare_ff_relative(x_used, x_available, 1.19209289550781250000000000000000000e-7F, 64)'

source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
1776

Looks like this debug print should be removed.

source/blender/makesdna/DNA_node_types.h
1452–1455

Include comments above the variables mentioning the enum types, like other structs above.

source/blender/makesrna/intern/rna_nodetree.c
572–578

These should have proper descriptions. Some initial suggestions based on the behavior I'm observing:

  • Overflow: Let the text use more space than the specified height
  • Scale to Fit: Scale the text size to fit inside the width and height
  • Truncate: Only output curves for the text that fits within the width and height. Output the remainder to the 'Truncated String' output.
576

Property names should be title case.

583–620

Maybe just copy the descriptions from the text object RNA here?

Are you using a debug build? I had some assert fail when changing the min width, in truncate mode I think.
BLI_assert failed: source/blender/blenkernel/intern/font.c:970, vfont_to_curve(), at 'compare_ff_relative(x_used, x_available, 1.19209289550781250000000000000000000e-7F, 64)'

Is that with or without rB56f8d7c70597: Fix T89241: Scale to fit overflows into a second line
?

See @Philipp Oeser (lichtwerk)’s comment here:
T91401#1219885

Erik Abrahamsson (erik85) marked 11 inline comments as done.

Rebased to compile on master branch and fixed review comments.

Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)

Rewritten to output instances of curves for each character instead of one single curve. This will make it much easier to manipulate the text further.

source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc
59

Does this mean we can no longer separate by lines?

Hans Goudey (HooglyBoogly) requested changes to this revision.Sep 20 2021, 4:46 AM
  • I'm getting an assert in truncate mode: BLI_assert failed: source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc:311, add_instances_from_component(), at 'charcodes.length() == domain_size'
    • Type asdf asdf
    • Switch to truncate mode
    • Make max width greater than 0
  • Increasing max width tends to move the characters to the right, when I think they should stay centered in this case
  • Jacques has been working on D12557: Geometry Nodes: New Transform Instances node (WIP). to transform instances. We discussed moving the per-character transforms there-- maybe that makes sense to do? It would definitely be nice to make this node simpler!
  • If we go this way, seems to me like we basically have something similar to the old named attribute workflow back here, but just so much better.
source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc
59

Right, that would wait until we support attributes on the instances component, which shouldn't be too hard.

113

const StringRef text

114

Floats can be passed by value

167–168

These aren't return variables, since they're local functions. Suggest text_size and text_free

170–171

Looking at this code path, it looks like the curve data is generated for every single character here, only to be created later again when you call BKE_vfont_build_char

The API is pretty awful, but I wonder if one of the options like FO_DUPLI calculate only the character positions to avoid the extra work here.

182
for (const int i : IndexRange(text_size)) {
  CharTrans &ct = char_trans_data[i];
  ...
}
205–206

Generally it's preferable to use StringRef instead of a const reference to a string. Though I won't speak for the strings of non-char types elsewhere in this file.

281

add_overwrite seems wrong here, since you already tested with contains above. We should really avoid generating the same character twice

305–306

I don't think this is true, this node creates an instances component from scratch.

This revision now requires changes to proceed.Sep 20 2021, 4:46 AM
Erik Abrahamsson (erik85) marked 7 inline comments as done.
Erik Abrahamsson (erik85) retitled this revision from [WIP] Geometry Nodes: Text nodes to Geometry Nodes: String to Curve.
Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)
  • Fixed all review comments.
  • Text now line-breaks at all characters above extended ascii.
Hans Goudey (HooglyBoogly) requested changes to this revision.Sep 21 2021, 11:54 PM
  • I think this is close, it's certainly nice how this patch keep getting simpler!
  • Pivot mode can be removed for now, we can add it back later when it's useful.
  • The fix for line wrapping with non-ascii characters should be a separate patch, not part of this one.
  • Mostly small comments about the code inline.
source/blender/makesrna/intern/rna_nodetree.c
571–666

These enums can be defined directly in def_geo_string_to_curve.
I think it's nice to decrease the scope like that, even if it makes the function longer.

576–587

The final periods are added to descriptions automatically and don't need to be added here.

10507–10511

The pivot point property doesn't do anything in the patch now, and should be removed.

source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc
139

I would suggest returning a struct like this rather than changing the input parameters:

struct TextLayout {
  Vector<float3> positions;

  /* The part of the input curve that didn't fit in the text box. Only set in truncate mode. */
  std::optional<StringRef> truncated_text;

  /* This should have a comment about what it means. */
  float size;
};

This way it's more clear what is an input and what is an output.

158–159

const

193

If you're just freeing the r_text after the function, do you have to give it as an argument? Or could you just pass null?

195

Add a comment about the FO_DUPLI argument, and the reason for that. (Did you verify that it does what I thought?)

205

Here you can reserve text_size size in the vector to avoid reallocation when it grows.

237

What's the runtime of std::string::replace? I wonder if it would be more efficient to build a separate string, since this might require moving the characters after the replacement?

242

Add a comment about the meaning of the map this function returns.

272

Is calling this necessary? I'm not sure what the purpose would be.

290–291

It seems simpler to avoid assigning a temporary value here.

306

Zero size is what the vector gets by default, no need to do that explicitly.

323

Pivot points are unused, the related functions and variables should be removed for now.

This revision now requires changes to proceed.Sep 21 2021, 11:54 PM
Erik Abrahamsson (erik85) marked 14 inline comments as done.
Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)

See review comments.

Hans Goudey (HooglyBoogly) requested changes to this revision.Sep 23 2021, 6:35 AM

It's so fast now with curve fill, this is great! Pretty close now, the only issue I found this time was this one with newlines:

Here are some smaller cleanups to this patch: P2423

  • Use float2 for position vector
  • Don't use unique_ptr for text layout, just return directly
  • Reorder some things, remove unnecessary checks
  • Add float4x4 from location constructor, use "apply_scale" instead of creating the matrix from a 0 rotation
source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc
101
258

Span is small (16 bytes) and should be passed by value, otherwise it adds another pointer indirection to element access (if not optimized away).

This revision now requires changes to proceed.Sep 23 2021, 6:35 AM
Erik Abrahamsson (erik85) marked 2 inline comments as done.
  • Applied patch supplied by Hans (thanks!)
  • Removed all special handling of the newline character. This simplifies the code and also means the input string length will match the number of instances added to the output.
Hans Goudey (HooglyBoogly) accepted this revision.EditedSep 24 2021, 7:09 AM

The code looks good now. The newline character patch is essential now, but that can be committed just before this.

I'll bring this up at the meeting tomorrow before committing it. Actually, if you could update the patch before with a comment about the size of the allocations, that would be great!

Nice job, this has really come a long way!

source/blender/makesrna/intern/rna_nodetree.c
10392

fits -> fit

I think "for the text" is obvious here and could be dropped to make the description shorter.

Also there is an unnecessary line break here.

source/blender/nodes/geometry/nodes/node_geo_string_to_curve.cc
22

The pointcloud includes are unused now.

161–162

Maybe it's worth adding a comment about + sizeof(char32_t) and + 4 here.

This revision is now accepted and ready to land.Sep 24 2021, 7:09 AM
Erik Abrahamsson (erik85) marked 2 inline comments as done.
Erik Abrahamsson (erik85) retitled this revision from Geometry Nodes: String to Curve to Geometry Nodes: String to Curves.
Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)

Renamed from String to Curve to String to Curves.

This revision was automatically updated to reflect the committed changes.

I made a couple changes when committing:

  • Hide max height in overflow mode, change socket label for width
  • Rename an enum to be a bit shorter
  • Add a comment about unknown reason for 1 character over-allocation.