VFONT: Text Selection Operator #106915

Merged
Harley Acheson merged 12 commits from Harley/blender:TextObjectSelection into main 2023-04-21 19:08:58 +02:00
Member

An operator to allow interactive text selection for 3D Text Objects.
This is from the code of Yash Dabhade (yashdabhade) for GSoC 2022
with corrections and simplifications. Also includes double-click for
word selection.


This started with Yash's code with Campbell's last review comments addressed, and simplified as much as possible. There are also some corrections so that it can no longer crash and it now handles changes to font size.

TextSelection.gif

TextSelectCurve.gif

This also allows selection of words by double-clicking:

TextWordSelect.gif

An operator to allow interactive text selection for 3D Text Objects. This is from the code of Yash Dabhade (yashdabhade) for GSoC 2022 with corrections and simplifications. Also includes double-click for word selection. --- This started with Yash's code with Campbell's last review comments addressed, and simplified as much as possible. There are also some corrections so that it can no longer crash and it now handles changes to font size. ![TextSelection.gif](/attachments/60eda4dd-d39d-4e27-b70f-5393dc52b932) ![TextSelectCurve.gif](/attachments/76fde7b2-e573-4ff6-810d-7409a42b9be9) This also allows selection of words by double-clicking: ![TextWordSelect.gif](/attachments/a503cf8e-db1b-48eb-8d06-757d5bbb8ea5)
Harley Acheson added 1 commit 2023-04-13 18:18:43 +02:00
00f41e79c7 WIP: VFONT: Text Selection Operator
An operator to allow interactive text selection for 3D Text Objects.
This is largely the code of Yash Dabhade (yashdabhade) for GSoC 2022
with corrections and simplifications.
Harley Acheson added this to the User Interface project 2023-04-13 18:22:41 +02:00
Campbell Barton requested changes 2023-04-14 04:23:13 +02:00
@ -780,2 +780,3 @@
bool *r_text_free,
struct CharTrans **r_chartransdata)
struct CharTrans **r_chartransdata,
float cursor_location[2],

Group return arguments last.

NOTE: I think it's a bit cleaner to add VFontCursor_Params struct which can be NULL. It can contain the 2D cursor location & the array offset r_caret. Normally I'd avoid a params being used for both input & output, but an exception could be made here as calculating the cursor placement is a special case and it avoids this function taking so many arguments (which is already getting a bit out of hand).

Group return arguments last. NOTE: I think it's a bit cleaner to add `VFontCursor_Params` struct which can be NULL. It can contain the 2D cursor location & the array offset `r_caret`. Normally I'd avoid a params being used for both input & output, but an exception could be made here as calculating the cursor placement is a special case and it avoids this function taking so many arguments (which is already getting a bit out of hand).
Harley marked this conversation as resolved
@ -781,1 +781,3 @@
struct CharTrans **r_chartransdata)
struct CharTrans **r_chartransdata,
float cursor_location[2],
int *r_caret_pos)

The term caret is a bit awkward - not used elsewhere in Blender, I have a slight preference to rename to cursor_offset or cursor_index, and accept that cursor_location is a different kind of cursor location.

The term `caret` is a bit awkward - not used elsewhere in Blender, I have a slight preference to rename to `cursor_offset` or `cursor_index`, and accept that `cursor_location` is a different kind of cursor location.
Harley marked this conversation as resolved
@ -914,0 +916,4 @@
int caret_pos = -1;
float caret_pos_min_dist = 0;
if (cursor_location && ef != NULL && ob != NULL) {

cursor_location && ef != NULL && ob != NULL is awkward. Prefer to use a boolean at the start of this function.
Assertions for a correct state can be there too, instead of in the middle of the function.

const bool calc_cursor_offset = ... optional cursor arg(s) ... ;

Then reuse for the rest of the function, otherwise it's not clear if it's expected that cursor_location is set but r_caret is null... (using a single cursor params arg would help, as suggested already).

`cursor_location && ef != NULL && ob != NULL` is awkward. Prefer to use a boolean at the start of this function. Assertions for a correct state can be there too, instead of in the middle of the function. `const bool calc_cursor_offset = ... optional cursor arg(s) ... ;` Then reuse for the rest of the function, otherwise it's not clear if it's expected that `cursor_location` is set but `r_caret` is null... (using a single cursor params arg would help, as suggested already).
Harley marked this conversation as resolved
@ -1128,0 +1139,4 @@
if (cursor_location) {
float offset_loc[2] = {cursor_location[0] - xof, cursor_location[1] - yof};
float di = len_squared_v2(offset_loc);

These names aren't so helpful, dist_sq_offset, could be: dist_sq_cursor.

These names aren't so helpful, `dist_sq_offset`, could be: `dist_sq_cursor`.
Harley marked this conversation as resolved
@ -1128,0 +1140,4 @@
if (cursor_location) {
float offset_loc[2] = {cursor_location[0] - xof, cursor_location[1] - yof};
float di = len_squared_v2(offset_loc);
float di2 = len_squared_v2(cursor_location);

Can't this be calculated once?

Can't this be calculated once?
Harley marked this conversation as resolved
@ -1799,0 +1824,4 @@
ef = cu->editfont;
BLI_assert(ef->len >= 0);
cur_loc_pos = BKE_vfont_cursor_to_caret_pos(ob, curs_loc);

Early exit in the case input motion sets the cursor motion to the same position it was set to previously (avoids excessive updates for small mouse-motions).

Early exit in the case input motion sets the cursor motion to the same position it was set to previously (avoids excessive updates for small mouse-motions).
Harley marked this conversation as resolved
Harley Acheson added 1 commit 2023-04-14 21:58:38 +02:00
Author
Member

@ideasman42 I think it's a bit cleaner to add VFontCursor_Params...

Yes, that cleaned up very nicely.

The term caret is a bit awkward...

I banished that word from here. Although that might be a nice refactor one day.

rename to cursor_offset or cursor_index, and accept that cursor_location is a different kind of cursor location

I sidestepped the issue by using "string_offset" instead.

> @ideasman42 I think it's a bit cleaner to add VFontCursor_Params... Yes, that cleaned up very nicely. > The term caret is a bit awkward... I banished that word from here. Although that might be a nice refactor one day. > rename to cursor_offset or cursor_index, and accept that cursor_location is a different kind of cursor location I sidestepped the issue by using "string_offset" instead.
Harley Acheson added 1 commit 2023-04-14 23:22:11 +02:00
Campbell Barton requested changes 2023-04-18 01:41:41 +02:00
@ -734,6 +734,14 @@ typedef struct VFontToCurveIter {
int status;
} VFontToCurveIter;
/* Used when translating a mouse cursor location to a position within the string. */

picky Prefer doxy-comments (/** ... */ for struct members too), write in full sentences.

*picky* Prefer doxy-comments (`/** ... */` for struct members too), write in full sentences.
Harley marked this conversation as resolved
@ -736,1 +736,4 @@
/* Used when translating a mouse cursor location to a position within the string. */
typedef struct VFontCursor_Params {
/* Mouse cursor location as input. */

Note what coordinate space this is in.

Note what coordinate space this is in.

Note what coordinate space this is in.

Note what coordinate space this is in.
Harley marked this conversation as resolved
@ -737,0 +738,4 @@
typedef struct VFontCursor_Params {
/* Mouse cursor location as input. */
float cursor_location[2];
/* string character position as output. */

Note what the offset is applied to (EditFont::textbuf , typically applied to EditFont::pos & selection range).

Note what the offset is applied to (`EditFont::textbuf` , typically applied to `EditFont::pos` & selection range).
Harley marked this conversation as resolved
@ -780,2 +788,3 @@
bool *r_text_free,
struct CharTrans **r_chartransdata)
struct CharTrans **r_chartransdata,
struct VFontCursor_Params *cursor_params)

Order r_ prefixed arguments last.

Order `r_` prefixed arguments last.
Harley marked this conversation as resolved
@ -912,2 +921,4 @@
i = 0;
float cursor_min_dist = 0;

Naming makes it seem this distance isn't squared, I normally name these *_best & *_test, cursor_sq_dist_best for e.g.

Naming makes it seem this distance isn't squared, I normally name these `*_best` & `*_test`, `cursor_sq_dist_best` for e.g.
@ -914,0 +925,4 @@
float cursor_sq_dist = 0;
if (cursor_params) {
cursor_params->r_string_offset = -1;
cursor_sq_dist = len_squared_v2(cursor_params->cursor_location);

Why is this set to anything at all? besides FLT_MAX or similar.

Why is this set to anything at all? besides `FLT_MAX or similar.`
@ -1128,0 +1148,4 @@
float offset_loc[2] = {cursor_params->cursor_location[0] - xof,
cursor_params->cursor_location[1] - yof};
float cursor_sq_offset = len_squared_v2(offset_loc);
if (cursor_sq_dist <= cursor_min_dist && cursor_sq_dist < cursor_sq_offset) {

This block could use some explanation, I'm not sure why it's necessary to reset the cursor offset when finding the closest cursor position.

It would seem to make sense to first set the cursor offset to an invalid value (-1) and the distance to FLT_MAX, then to simply calculate to closest cursor position. There may be good reason not to do this, but in that case the reason should be noted in comments.

This block could use some explanation, I'm not sure why it's necessary to reset the cursor offset when finding the closest cursor position. It would seem to make sense to first set the cursor offset to an invalid value (-1) and the distance to FLT_MAX, then to simply calculate to closest cursor position. There may be good reason not to do this, but in that case the reason should be noted in comments.
@ -1799,0 +1824,4 @@
ef = cu->editfont;
BLI_assert(ef->len >= 0);
cur_loc_pos = BKE_vfont_cursor_to_string_offset(ob, curs_loc);

picky can set a const int here.

*picky* can set a `const int` here.
Harley marked this conversation as resolved
@ -1799,0 +1871,4 @@
static int font_selection_set_modal(bContext *C, wmOperator *UNUSED(op), const wmEvent *event)
{
switch (event->type) {
case LEFTMOUSE: // release

May as well check value is release in this case.

May as well check value is release in this case.
Harley marked this conversation as resolved
Harley Acheson added 1 commit 2023-04-18 18:33:47 +02:00
Harley Acheson added 1 commit 2023-04-19 23:57:08 +02:00
Author
Member

@ideasman42 - Naming makes it seem this distance isn't squared, I normally name these *_best & *_test

Did so.

Why is this set to anything at all? besides FLT_MAX or similar

Yes, indeed works with just FLT_MAX

This block could use some explanation

I did manage to simplify it a bit more to just the needed parts, added some comments on the behavior but otherwise couldn't do much more with it.

Fixed a small bug that was hard to notice. If the very first touch down was on the left side of the first character this would not set and show the cursor. Subsequent interaction would work and and select that first character, just not that first touch down. But fixed.

> @ideasman42 - Naming makes it seem this distance isn't squared, I normally name these *_best & *_test Did so. > Why is this set to anything at all? besides FLT_MAX or similar Yes, indeed works with just FLT_MAX > This block could use some explanation I did manage to simplify it a bit more to just the needed parts, added some comments on the behavior but otherwise couldn't do much more with it. Fixed a small bug that was hard to notice. If the very first touch down was on the left side of the first character this would not set and show the cursor. Subsequent interaction would work and and select that first character, just not that first touch down. But fixed.
Campbell Barton requested changes 2023-04-20 01:35:17 +02:00
@ -73,2 +73,4 @@
struct CharTrans **r_chartransdata);
bool BKE_vfont_to_curve_nubase(struct Object *ob, int mode, struct ListBase *r_nubase);
int BKE_vfont_cursor_to_string_offset(struct Object *ob, float cursor_location[2]);

Naming could still be clearer, both input and output are cursors and both can have offsets.

The term string could be text renamed to text, as this is what the edit-font member is called.

Suggest: BKE_vfont_cursor_to_text_index to make it clear the output is an index in an array (any coordinate space can have an offset).

Naming could still be clearer, both input and output are cursors and both can have offsets. The term `string` could be text renamed to `text`, as this is what the edit-font member is called. Suggest: `BKE_vfont_cursor_to_text_index` to make it clear the output is an index in an array (any coordinate space can have an offset).
Harley marked this conversation as resolved
@ -914,0 +933,4 @@
float cursor_sq_dist_test = 0;
if (cursor_params) {
cursor_params->r_string_offset = -1;
cursor_sq_dist_test = len_squared_v2(cursor_params->cursor_location);

Why is this distance relevant? From what I can see only the distance to the characters should have to be measured. The distance from the mouse cursor the objects center doesn't seem like it should be used.

Text boxes can offset text so the absolute distance to the object center doesn't seem relevant.

Why is this distance relevant? From what I can see only the distance to the characters should have to be measured. The distance from the mouse cursor the objects center doesn't seem like it should be used. Text boxes can offset text so the absolute distance to the object center doesn't seem relevant.
Harley marked this conversation as resolved
@ -1128,0 +1162,4 @@
}
else if (cursor_sq_dist_test < cursor_sq_dist_best) {
/* Left of text, including left-side of first character. */
cursor_params->r_string_offset = -1;

Checking if the cursor is to the left should probably not be done with distances (I'd guess this would fail with text-box offsets) and instead check if the cursor is actually to the left of all characters. Although I'm not even sure this is needed. Perhaps for an initial pass just get basics working and worry about moving the cursor outside text regions after since it could get quite involved with multiple text boxes.

Checking if the cursor is to the left should probably not be done with distances (I'd guess this would fail with text-box offsets) and instead check if the cursor is actually to the left of all characters. Although I'm not even sure this is needed. Perhaps for an initial pass just get basics working and worry about moving the cursor outside text regions after since it could get quite involved with multiple text boxes.
Harley marked this conversation as resolved
@ -1799,0 +1806,4 @@
Curve *cu = obedit->data;
EditFont *ef = cu->editfont;
/* Calculate a plane from the text object's orientation. */

A utility function would be clearer, font_cursor_text_index_from_event(...) for e.g. to split out the plane-projection from the text offset logic.

A utility function would be clearer, `font_cursor_text_index_from_event(...)` for e.g. to split out the plane-projection from the text offset logic.
Harley marked this conversation as resolved
Harley Acheson added 1 commit 2023-04-20 04:40:48 +02:00
Author
Member

@ideasman42

Okay I officially give up trying to figure out what Yash was doing there, so dug out all the "squared distance" stuff. The code before the per-character loop is all removed and then I just wrote simpler code for finding the string offset. Could probably be made prettier but seems to test okay.

What it does not do though, and I miss a bit, is being able to start a drag to the left or right of all the text. This requires selection of the text itself which seems okay. Maybe is better since it doesn't involve clicks outside the text so won't interfere as much with 3D cursor placement?

@ideasman42 Okay I officially give up trying to figure out what Yash was doing there, so dug out all the "squared distance" stuff. The code before the per-character loop is all removed and then I just wrote simpler code for finding the string offset. Could probably be made prettier but seems to test okay. What it does not do though, and I miss a bit, is being able to start a drag to the left or right of all the text. This requires selection of the text itself which seems okay. Maybe is better since it doesn't involve clicks outside the text so won't interfere as much with 3D cursor placement?
Campbell Barton approved these changes 2023-04-20 04:47:55 +02:00
@ -737,0 +741,4 @@
* This is an optional argument to `vfont_to_curve` for getting the text
* offset into the string at a mouse cursor location. Used for getting
* text cursor (caret) position or selection range.
* \{ */

Missed a closing /** \} */ comment - both before and when the group ends.

Missed a closing `/** \} */` comment - both before and when the group ends.
Harley marked this conversation as resolved
@ -911,6 +928,7 @@ static bool vfont_to_curve(Object *ob,
}
i = 0;

Can leave as-is.

Can leave as-is.
Harley marked this conversation as resolved
@ -1796,6 +1796,148 @@ void FONT_OT_text_insert(wmOperatorType *ot)
/** \} */
static void font_cursor_text_index_from_event(bContext *C,

This can return the index from BKE_vfont_cursor_to_text_index as the caller doesn't need to know the intermediate value.

This can return the index from `BKE_vfont_cursor_to_text_index` as the caller doesn't need to know the intermediate value.
Harley marked this conversation as resolved
@ -1799,0 +1820,4 @@
r_curs_loc[1] = mouse_loc[1] / cu->fsize;
}
/* -------------------------------------------------------------------- */

The group can enclose the utility function (doesn't seem needed to define a new group).

The group can enclose the utility function (doesn't seem needed to define a new group).
Harley marked this conversation as resolved
Campbell Barton reviewed 2023-04-20 04:51:41 +02:00
@ -1128,0 +1146,4 @@
if (cursor_params) {
float midw = (twidth / 2.0f);
if (cursor_params->cursor_location[1] >= yof - (0.25f * linedist) &&

This probably wont work with text-on-curve, suggest to avoid all bounding box checks unless there very well tested with different configurations. For initial working selection I don't think they're needed.

This probably wont work with text-on-curve, suggest to avoid all bounding box checks unless there very well tested with different configurations. For initial working selection I don't think they're needed.
Harley marked this conversation as resolved
Campbell Barton requested changes 2023-04-20 04:56:58 +02:00
Campbell Barton left a comment
Owner

Requesting changes as key-map conflict with 3D cursor isn't resolved yet.

Requesting changes as key-map conflict with 3D cursor isn't resolved yet.
Harley Acheson added 1 commit 2023-04-20 18:36:10 +02:00
Harley Acheson added 1 commit 2023-04-20 18:38:35 +02:00
Author
Member

@ideasman42 - This probably wont work with text-on-curve, suggest to avoid all bounding box checks...

Got it! There was a fundamental flaw in Yash's approach of comparing against the mouse position within that loop that creates each character: After that loop is complete those character positions are translated by horizontal alignment, vertical alignment, and by Text on Curve.

This now just checks mouse position against each character's position after the translating is all done. So this works with all alignments and with Text on Curve.

> @ideasman42 - This probably wont work with text-on-curve, suggest to avoid all bounding box checks... Got it! There was a fundamental flaw in Yash's approach of comparing against the mouse position within that loop that creates each character: After that loop is complete those character positions are translated by horizontal alignment, vertical alignment, and by Text on Curve. This now just checks mouse position against each character's position after the translating is all done. So this works with all alignments and with Text on Curve.
Harley Acheson changed title from WIP: VFONT: Text Selection Operator to VFONT: Text Selection Operator 2023-04-20 19:00:50 +02:00

Getting a build error:

/src/blender/source/blender/blenkernel/intern/vfont.c: In function 'BKE_vfont_to_curve_ex':
/src/blender/source/blender/blenkernel/intern/vfont.c:1790:16: error: too many arguments to function 'vfont_to_curve'
 1790 |     data.ok &= vfont_to_curve(ob,
      |                ^~~~~~~~~~~~~~
/src/blender/source/blender/blenkernel/intern/vfont.c:792:13: note: declared here
  792 | static bool vfont_to_curve(Object *ob,
      |             ^~~~~~~~~~~~~~

Getting a build error: ``` /src/blender/source/blender/blenkernel/intern/vfont.c: In function 'BKE_vfont_to_curve_ex': /src/blender/source/blender/blenkernel/intern/vfont.c:1790:16: error: too many arguments to function 'vfont_to_curve' 1790 | data.ok &= vfont_to_curve(ob, | ^~~~~~~~~~~~~~ /src/blender/source/blender/blenkernel/intern/vfont.c:792:13: note: declared here 792 | static bool vfont_to_curve(Object *ob, | ^~~~~~~~~~~~~~ ```
Harley Acheson added 2 commits 2023-04-21 05:53:58 +02:00
Campbell Barton approved these changes 2023-04-21 05:55:48 +02:00
@ -5611,6 +5611,7 @@ def km_font(params):
{"properties": [("delta", 1)]}),
("font.change_character", {"type": 'DOWN_ARROW', "value": 'PRESS', "alt": True, "repeat": True},
{"properties": [("delta", -1)]}),
("font.select_word", {"type": 'LEFTMOUSE', "value": 'DOUBLE_CLICK'}, None),

Better make this part of the tools key-map too. Seems strange this would be accessible without regular selection.

Better make this part of the tools key-map too. Seems strange this would be accessible without regular selection.
Harley marked this conversation as resolved
Campbell Barton reviewed 2023-04-21 06:08:15 +02:00
@ -1262,0 +1269,4 @@
cursor='CROSSHAIR',
icon="ops.generic.select_box",
widget=None,
keymap=("3D View Tool: Text Select"),

This should be left an empty tuple for the name to be generated (as is done with other tools).

This should be left an empty tuple for the name to be generated (as is done with other tools).
Harley marked this conversation as resolved
Harley Acheson added 1 commit 2023-04-21 18:36:28 +02:00
Harley Acheson added 1 commit 2023-04-21 18:56:41 +02:00
Harley Acheson merged commit 68f8253c71 into main 2023-04-21 19:08:58 +02:00
Harley Acheson deleted branch TextObjectSelection 2023-04-21 19:08:59 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106915
No description provided.