Refactoring: Corrections and unifications in mathematics vfont gizmos #107193

Iliya Katushenock · 2023-04-20T21:01:54+02:00

Iliya Katushenock commented

2023-04-20 21:01:54 +02:00

Goals:

Simplified code of defining cursor shape and unified this with selection box gizmos.
Fixing deformation of cursor.
Fix of symbol and selection collision due to rotation of this.
More context-dependent align cursor to close symbol.

Changes:

Delete textcurs[4][2].
Unified naming: loc, size, rot for all transforms.
The gizmo transform structure can be reused for both selection and cursor. If ever they will be start maintaining the deformation, then both will do this it the same way.

Comparison:

	Main	RP
Cursor deformation
Selection box shifting
Different cursor and selection behavior
Сontext-dependent align

More aboud context-depend align cursore:

	Main	RP
To close word (avoid align to space)
Attach to start or end of current line (avoid align to newline char)
Align to selection if it exist

Goals: 1. Simplified code of defining cursor shape and unified this with selection box gizmos. 2. Fixing deformation of cursor. 3. Fix of symbol and selection collision due to rotation of this. 4. More context-dependent align cursor to close symbol. Changes: 1. Delete `textcurs[4][2]`. 2. Unified naming: `loc`, `size`, `rot` for all transforms. 3. The gizmo transform structure can be reused for both selection and cursor. If ever they will be start maintaining the deformation, then both will do this it the same way. ## Comparison: | | Main | RP | | -- | -- | -- | | Cursor deformation | ![image](/attachments/5a70350e-e312-4115-a2ca-17f41d42a2d4) | ![image](/attachments/354eea4b-1571-4ce4-8770-6ee342bf7015) | | Selection box shifting | ![image](/attachments/83bb1a92-8d1c-4b47-868f-b3c41a73d03c) | ![image](/attachments/bf04a08a-a5e7-42ba-aa27-c6e83da6edec) | | Different cursor and selection behavior | ![image](/attachments/69b8dedc-c031-4dae-ba56-c795d583fa41) | ![image](/attachments/98c13ddc-c5e0-428c-a7c9-aba18ac338eb) | | Сontext-dependent align | ![image](/attachments/7a54e4fb-63dd-4c47-81c1-c910fd524186) | ![image](/attachments/479cfa50-322e-4bb2-b5bb-72299a5c090c) | ## More aboud context-depend align cursore: | | Main | RP | | -- | -- | -- | | To close word (avoid align to space)| ![image](/attachments/f0355f06-99d7-4c38-93e5-9d8b42553de0) | ![image](/attachments/a1030d9f-5b02-4f0c-8009-b7c9d1d40230) | | Attach to start or end of current line (avoid align to newline char) | ![image](/attachments/73681fcd-caca-4105-bbdd-c9379a5b518c) | ![image](/attachments/5c7c0c7f-c62c-4037-b9d6-7712f30750d3) | | Align to selection if it exist | ![image](/attachments/6b4a906a-ba5f-4139-9c5a-d0f6617d579d) | ![image](/attachments/6eeca426-ffec-4339-bf09-e4dba0e0c06e) |

image.png

79 KiB

image.png

94 KiB

image.png

38 KiB

image.png

90 KiB

image.png

88 KiB

image.png

39 KiB

image.png

119 KiB

image.png

105 KiB

image.png

106 KiB

image.png

91 KiB

image.png

116 KiB

image.png

47 KiB

image.png

44 KiB

image.png

75 KiB

Iliya Katushenock added 1 commit 2023-04-20 21:02:04 +02:00

e80c2d83e4 init

Iliya Katushenock added this to the User Interface project 2023-04-20 21:04:58 +02:00

Iliya Katushenock requested review from Campbell Barton 2023-04-20 21:05:15 +02:00

Harley Acheson commented

2023-04-20 21:34:27 +02:00

This looks to be a nice start, but needs more testing.

At first glance this appears to nicely improve many issues with the size, placement, and rotation with "Text on Curve".

But try it with regular text that is not on a curve and it is worse than now. Try it with a text string like "MiMiMi" and you'll notice the placement is way out. There are also horizontal placement issues if you test with text that wraps to multiple lines.

This looks to be a nice start, but needs more testing. At first glance this appears to nicely improve many issues with the size, placement, and rotation with "Text on Curve". But try it with regular text that is not on a curve and it is worse than now. Try it with a text string like "MiMiMi" and you'll notice the placement is way out. There are also horizontal placement issues if you test with text that wraps to multiple lines.

👍 2

Iliya Katushenock added 2 commits 2023-04-21 00:47:34 +02:00

cddb46d4d9 Merge branch 'main' into tmp_fix_text_cursor_transform

cf33701ccc fix lost offset scaling

Harley Acheson commented

2023-04-21 01:32:42 +02:00

Hey, that is working way better now.

Although a bit picky, current code shows the text cursor (caret) lining up vertically perfectly with the selection boxes (left), but are out by a bit with your PR:

An optionally... just a thought. But wondering if the angle used should actually be the average of the character and the previous (if there is one). Might be nicer with extreme bends? Or might be worse? Not sure.

Although maybe it would make more sense to first better align neighboring select boxes and then see how the mouse cursor fits in.

Hey, that is working way better now. Although a bit picky, current code shows the text cursor (caret) lining up _vertically_ perfectly with the selection boxes (left), but are out by a bit with your PR: ![image](/attachments/31a2e662-a276-4fc2-b503-ed2d848bcb39) An optionally... just a thought. But wondering if the angle used should actually be the average of the character and the previous (if there is one). Might be nicer with extreme bends? Or might be worse? Not sure. ![image](/attachments/cc892296-f7f6-4118-ac08-0943f1936c11) Although maybe it would make more sense to first better align neighboring select boxes and then see how the mouse cursor fits in. ![image](/attachments/9bb4d33a-3555-4e44-b4c2-820be722750d)

image.png

91 KiB

image.png

67 KiB

image.png

115 KiB

Iliya Katushenock commented

2023-04-21 09:53:36 +02:00

@Harley Thanks for your testing this

Although a bit picky, current code shows the text cursor (caret) lining up vertically perfectly with the selection boxes (left), but are out by a bit with your PR:

I was not able to determine the real ratios of the high and low points using the old formulas (because it would be difficult to understand what this should mean). But the fact that they are not similar to other gizmos, I did not notice this.

An optionally... just a thought. But wondering if the angle used should actually be the average of the character and the previous (if there is one). Might be nicer with extreme bends? Or might be worse? Not sure.

There seem to be 2 options here:

The font is slanted. Here i need to do a shift of vertical points.
The curve is strongly curved. It is necessary to radially split the upper points.

Although maybe it would make more sense to first better align neighboring select boxes and then see how the mouse cursor fits in.

Yes, it looks like I should also take a look at the code for other parts of the gizmo so that all the calculations are more consistent.

@Harley Thanks for your testing this > Although a bit picky, current code shows the text cursor (caret) lining up vertically perfectly with the selection boxes (left), but are out by a bit with your PR: I was not able to determine the real ratios of the high and low points using the old formulas (because it would be difficult to understand what this should mean). But the fact that they are not similar to other gizmos, I did not notice this. > An optionally... just a thought. But wondering if the angle used should actually be the average of the character and the previous (if there is one). Might be nicer with extreme bends? Or might be worse? Not sure. There seem to be 2 options here: 1. The font is slanted. Here i need to do a shift of vertical points. 2. The curve is strongly curved. It is necessary to radially split the upper points. > Although maybe it would make more sense to first better align neighboring select boxes and then see how the mouse cursor fits in. Yes, it looks like I should also take a look at the code for other parts of the gizmo so that all the calculations are more consistent.

Iliya Katushenock requested review from Harley Acheson 2023-04-21 09:53:48 +02:00

Iliya Katushenock changed title from ~~VFONT: Fix cursor deforming on transformation~~ to WIP: VFONT: Fix cursor deforming on transformation

2023-04-22 12:07:30 +02:00

Iliya Katushenock added 4 commits 2023-04-22 17:42:21 +02:00

cb95789a7f Merge branch 'main' into tmp_fix_text_cursor_transform

267fabcd06 progress

42d306f377 Merge branch 'main' into tmp_fix_text_cursor_transform

ea8853a8d3 progress

Harley Acheson commented

2023-04-22 20:29:56 +02:00

Wow that does work nicely. Is that much change really necessary though?

From what I can tell the fundamental problem in current code is simply that the positions of the text insertion cursor are rotated incorrectly. If you remove the current rotation adjustments you get a perfect cursor on a flat surface. But place it on a curve and the points are rotated incorrectly.

The way the characters are rotated, the bottom-left point is in the correct position, so the cursor needs to be rotated around that point, not the middle of it. I get quite a good result with just doing that:

diff --git a/source/blender/blenkernel/intern/vfont.c b/source/blender/blenkernel/intern/vfont.c
index c0a9a1b5854..a0858ac9e5b 100644
--- a/source/blender/blenkernel/intern/vfont.c
+++ b/source/blender/blenkernel/intern/vfont.c
@@ -1536,25 +1536,34 @@ static bool vfont_to_curve(Object *ob,
 
   /* Cursor first. */
   if (ef) {
-    float si, co;
-
     ct = &chartransdata[ef->pos];
-    si = sinf(ct->rot);
-    co = cosf(ct->rot);
-
-    f = ef->textcurs[0];
-
-    f[0] = font_size * (-0.02f * co + ct->xof);
-    f[1] = font_size * (0.1f * si - (0.25f * co) + ct->yof);
-
-    f[2] = font_size * (0.02f * co + ct->xof);
-    f[3] = font_size * (-0.1f * si - (0.25f * co) + ct->yof);
-
-    f[4] = font_size * (0.02f * co + 0.8f * si + ct->xof);
-    f[5] = font_size * (-0.1f * si + 0.75f * co + ct->yof);
-
-    f[6] = font_size * (-0.02f * co + 0.8f * si + ct->xof);
-    f[7] = font_size * (0.1f * si + 0.75f * co + ct->yof);
+    float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot;
+    float width = 0.02f;
+    float height = 1.0f;
+
+    /* Bottom left. */
+    ef->textcurs[0][0] = -width;
+    ef->textcurs[0][1] = 0.0f;
+
+    /* Bottom right. */
+    ef->textcurs[1][0] = width;
+    ef->textcurs[1][1] = 0.0f;
+
+    /* Top right. */
+    ef->textcurs[2][0] = width;
+    ef->textcurs[2][1] = height;
+
+    /* Top left. */
+    ef->textcurs[3][0] = -width;
+    ef->textcurs[3][1] = height;
+
+    for (int i = 0; i < 4; i++) {
+      float temp_fl[2];
+      rotate_v2_v2fl(temp_fl, &ef->textcurs[i][0], -rot);
+      ef->textcurs[i][0] = temp_fl[0] + ct->xof;
+      ef->textcurs[i][1] = temp_fl[1] + ct->yof - 0.25f;
+      mul_v2_fl(&ef->textcurs[i][0], font_size);
+    }
   }
 
   if (mode == FO_SELCHANGE) {

Wow that does work nicely. Is that much change really necessary though? From what I can tell the fundamental problem in current code is simply that the positions of the text insertion cursor are _rotated incorrectly_. If you **remove** the current rotation adjustments you get a perfect cursor on a flat surface. But place it on a curve and the points are rotated incorrectly. The way the characters are rotated, the bottom-left point is in the correct position, so the cursor needs to be rotated around that point, not the middle of it. I get quite a good result with just doing that: ```Diff diff --git a/source/blender/blenkernel/intern/vfont.c b/source/blender/blenkernel/intern/vfont.c index c0a9a1b5854..a0858ac9e5b 100644 --- a/source/blender/blenkernel/intern/vfont.c +++ b/source/blender/blenkernel/intern/vfont.c @@ -1536,25 +1536,34 @@ static bool vfont_to_curve(Object *ob, /* Cursor first. */ if (ef) { - float si, co; - ct = &chartransdata[ef->pos]; - si = sinf(ct->rot); - co = cosf(ct->rot); - - f = ef->textcurs[0]; - - f[0] = font_size * (-0.02f * co + ct->xof); - f[1] = font_size * (0.1f * si - (0.25f * co) + ct->yof); - - f[2] = font_size * (0.02f * co + ct->xof); - f[3] = font_size * (-0.1f * si - (0.25f * co) + ct->yof); - - f[4] = font_size * (0.02f * co + 0.8f * si + ct->xof); - f[5] = font_size * (-0.1f * si + 0.75f * co + ct->yof); - - f[6] = font_size * (-0.02f * co + 0.8f * si + ct->xof); - f[7] = font_size * (0.1f * si + 0.75f * co + ct->yof); + float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot; + float width = 0.02f; + float height = 1.0f; + + /* Bottom left. */ + ef->textcurs[0][0] = -width; + ef->textcurs[0][1] = 0.0f; + + /* Bottom right. */ + ef->textcurs[1][0] = width; + ef->textcurs[1][1] = 0.0f; + + /* Top right. */ + ef->textcurs[2][0] = width; + ef->textcurs[2][1] = height; + + /* Top left. */ + ef->textcurs[3][0] = -width; + ef->textcurs[3][1] = height; + + for (int i = 0; i < 4; i++) { + float temp_fl[2]; + rotate_v2_v2fl(temp_fl, &ef->textcurs[i][0], -rot); + ef->textcurs[i][0] = temp_fl[0] + ct->xof; + ef->textcurs[i][1] = temp_fl[1] + ct->yof - 0.25f; + mul_v2_fl(&ef->textcurs[i][0], font_size); + } } if (mode == FO_SELCHANGE) { ```

Iliya Katushenock commented

2023-04-22 20:49:52 +02:00

The main change is the refusal to use a rectangle of points.
As far as I can tell, making a gizmo deform when the text doesn't deform on curve is not very good. In addition, the render itself, at least in the current implementation, does not support this anyway.
So I simplified the logic by just starting to use transform. Otherwise, I just unify the processing of the transformation and its definition.
From your code: A bit unsure of its meaning, the line:
float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot;
Can be simplified to:
float rot = true ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot;
float rot = (ct->rot + chartransdata[ef->pos].rot) / 2.0f
float rot = (ct->rot + ct->rot) / 2.0f
float rot = ct->rot
Which in fact is not so far from the current version.

All symbols, gizmos and cursors have one pivot point. This is {0.5f, 0.0f}, the center of the bottom line. This is where I understand the symbol touches the curve (I could be wrong).
So, the gizmo of the character and the cursor must be in this space.
If the cursor is not next to the selection (that is, there is no selection), then yes, i need to position it in the center, which is similar to your code:
float rot = (ef->pos < size) ? ((ct->rot + chartransdata[ef->pos + 1].rot) / 2.0f) : ct->rot;
But it hasn't been added yet (look for the big commented else)

The main change is the refusal to use a rectangle of points. As far as I can tell, making a gizmo deform when the text doesn't deform on curve is not very good. In addition, the render itself, at least in the current implementation, does not support this anyway. So I simplified the logic by just starting to use transform. Otherwise, I just unify the processing of the transformation and its definition. From your code: A bit unsure of its meaning, the line: `float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot;` Can be simplified to: `float rot = true ? ((ct->rot + chartransdata[ef->pos].rot) / 2.0f) : ct->rot;` `float rot = (ct->rot + chartransdata[ef->pos].rot) / 2.0f` `float rot = (ct->rot + ct->rot) / 2.0f` `float rot = ct->rot` Which in fact is not so far from the current version. All symbols, gizmos and cursors have one pivot point. This is {0.5f, 0.0f}, the center of the bottom line. This is where I understand the symbol touches the curve (I could be wrong). So, the gizmo of the character and the cursor must be in this space. If the cursor is not next to the selection (that is, there is no selection), then yes, i need to position it in the center, which is similar to your code: `float rot = (ef->pos < size) ? ((ct->rot + chartransdata[ef->pos + 1].rot) / 2.0f) : ct->rot;` But it hasn't been added yet (look for the big commented `else`)

Harley Acheson commented

2023-04-22 20:56:57 +02:00

From your code: A bit unsure of its meaning, the line...

Yikes, just a brain-fart. Change that to the following and you'll see I was just wanting to use the average rotation of the two neighboring characters (as long as we have a neighbor). Not sure if it helps much though.

float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos - 1].rot) / 2.0f) : ct->rot;

> From your code: A bit unsure of its meaning, the line... Yikes, just a brain-fart. Change that to the following and you'll see I was just wanting to use the average rotation of the two neighboring characters (as long as we have a neighbor). Not sure if it helps much though. ```C float rot = (ef->pos > 0) ? ((ct->rot + chartransdata[ef->pos - 1].rot) / 2.0f) : ct->rot; ```

👀 1

Iliya Katushenock added 2 commits 2023-04-23 01:04:48 +02:00

6a224e7949 Merge branch 'main' into tmp_fix_text_cursor_transform

afa64e842b progress

Iliya Katushenock added 2 commits 2023-04-23 14:05:34 +02:00

2969116674 Merge branch 'main' into tmp_fix_text_cursor_transform

9fec2dd185 progress

Iliya Katushenock added 1 commit 2023-04-23 14:29:47 +02:00

9adb0a1fe6 progress

Iliya Katushenock changed title from ~~WIP: VFONT: Fix cursor deforming on transformation~~ to Refactoring: Corrections and unifications in mathematics vfont gizmos

2023-04-23 14:34:53 +02:00

Iliya Katushenock added 1 commit 2023-04-23 15:26:27 +02:00

bedafa141a progress

Iliya Katushenock added 1 commit 2023-04-23 15:38:50 +02:00

6928d95478 progress

Iliya Katushenock added 2 commits 2023-04-24 18:24:19 +02:00

07b51b0799 Merge branch 'main' into tmp_fix_text_cursor_transform

a4cb14b473 progress

Harley Acheson referenced this pull request

2023-04-24 19:13:11 +02:00

VFont: Corrections to Cursor When on Curve #107307

Iliya Katushenock added 1 commit 2023-04-24 20:11:34 +02:00

fcc281a6c0 replace geom by rotate_v2_v2f

Iliya Katushenock added 2 commits 2023-04-25 18:44:35 +02:00

7c3558b449 Merge branch 'main' into tmp_fix_text_cursor_transform

07f381c815 fix comment

Iliya Katushenock commented

2023-04-25 19:30:19 +02:00

Added some more description with examples.

Campbell Barton commented

2023-05-01 02:02:47 +02:00

Some issues with this PR fixes look to be resolved in main.

This PR would need to be updated, but I'm not convinced of these changes.

Does this PR fixes any remaining bugs? If so, they can be extracted into a separate PR.
The refactor to merge the cursor & text box data structure seems OK and could be made into a separate PR.
Interpolating rotation of the cursor between characters is a detail, nice but quite low priority as this is only noticeable in extreme cases.

So while I'm not against these changes, they don't seem so important - but I may be missing something.

Some issues with this PR fixes look to be resolved in `main`. - 1d2eab9ab3a18347bd6fda35d0b3c9ca323424fc - 5c184525ed59822b37490b34337d66683925bdf1 This PR would need to be updated, but I'm not convinced of these changes. - Does this PR fixes any remaining bugs? If so, they can be extracted into a separate PR. - The refactor to merge the cursor & text box data structure seems OK and could be made into a separate PR. - Interpolating rotation of the cursor between characters is a detail, nice but quite low priority as this is only noticeable in extreme cases. So while I'm not against these changes, they don't seem so important - but I may be missing something.

👍 1

Iliya Katushenock closed this pull request

2023-05-01 07:44:22 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

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

Refactoring: Corrections and unifications in mathematics vfont gizmos #107193

Comparison:

More aboud context-depend align cursore:

Pull request closed