-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support path function to fulfill TinyVG rendering #74
base: main
Are you sure you want to change the base?
Conversation
8f7ace6
to
fe63524
Compare
src/trig.c
Outdated
int quadrant; | ||
twin_fixed_t abs_x = x; | ||
twin_fixed_t abs_y = y; | ||
if (x >= 0 && y >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it can be modified to a branch-free method for determining the quadrant of the angle.
The return values seem to follow a certain pattern, which can be expressed in the following algebraic form as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sign variable, there is a more elegant approach using bitwise operations. Similarly, the absolute values of x
and y
can also be simplified through bitwise manipulation, streamlining the entire computation.
To achieve the method described above, two key concepts are essential:
- the two's complement system
-x = ~x + 1
and - XOR operation.
From the two's complement system, we know that −x = ∼x + 1 = x ^ -1 - (-1)
. Using the definition of the absolute value function:
This can be rewritten using the two's complement system as:
At the same time, -1
in the two's complement system is represented as 0xFFFFFFFF
, which serves as the sign bit mask when x
is a negative number.
By using x >> 31
, the sign bit mask can be obtained. For example:
- When
x
is negative,x >> 31
results in0xFFFFFFFF
. - When
x
is non-negative,x >> 31
results in0x00000000
.
Based on the above discussion, the absolute value of x
can be implemented in a branch-free version using the sign bit mask.
x_sign_mask = x >> 31;
abs_x = (x ^ x_sign_mask) - x_sign_mask;
Next, we can determine m
based on the sign bit mask, and it can be implemented in a branch-free version.
Here, we use some auxiliary functions m
.
where the functions
The above auxiliary functions exist and can be used to represent m
as
m = ((~x_sign_mask & ~y_sign_mask ) * 0) +
((x_sign_mask & ~y_sign_mask ) * 1) +
((x_sign_mask & y_sign_mask ) * 1) +
((~x_sign_mask & y_sign_mask ) * 2);
you can substitute simple values to verify the calculation.
Finally, the sign
can also be determined using a branch-free method as following
sign = 1 - 2 * (x_sign_mask^ y_sign_mask);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you still have no idea to implement, ChatGPT is a good helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jouae Thanks for the detailed explanation : )
twin_int_to_fixed(2)); | ||
twin_path_empty(path); | ||
for (i = 0; i < NPT; i++) { | ||
if (spline->n_points == 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you generalize the handling for n_points = 3 and 4? That is, share code by avoiding duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set your username in Git, and use git commit --amend --author
to change your name to your formal name in Pinyin mentioned in your resume. This helps others recognize your identity and acknowledge your contributions.
twin_sfixed_t dy2 = twin_fixed_to_sfixed(y1) - y2s; | ||
twin_sfixed_t cx2 = x2s + twin_sfixed_mul(dx2, twin_int_to_sfixed(2) / 3); | ||
twin_sfixed_t cy2 = y2s + twin_sfixed_mul(dy2, twin_int_to_sfixed(2) / 3); | ||
_twin_path_scurve(path, cx1, cy1, cx2, cy2, x2s, y2s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m curious why the function twin_path_quadratic_curve()
does not use the function _twin_matrix_x()
and _twin_matrix_y()
.
Another question: Is there any difference compared to directly calculating the values x1
, y1
, x2
, and y2
, which are of type twin_fixed_t
, and then calling twin_path_curve(path, x1, y1, x2, y2, x3, y3)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because operations with twin_fixed_t
are too complex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another way to write this twin_path_quadratic_curve()
.
void twin_path_quadratic_curve(twin_path_t *path,
twin_fixed_t x1,
twin_fixed_t y1,
twin_fixed_t x2,
twin_fixed_t y2)
{
twin_spoint_t p0 = _twin_path_current_spoint(path);
twin_fixed_t x1_ratio = twin_fixed_div(
twin_fixed_mul(x1, twin_int_to_fixed(2)), twin_int_to_fixed(3));
twin_fixed_t y1_ratio = twin_fixed_div(
twin_fixed_mul(y1, twin_int_to_fixed(2)), twin_int_to_fixed(3));
return twin_path_curve(
path,
twin_fixed_div(twin_sfixed_to_fixed(p0.x), twin_int_to_fixed(3)) +
x1_ratio,
twin_fixed_div(twin_sfixed_to_fixed(p0.y), twin_int_to_fixed(3)) +
y1_ratio,
x1_ratio + twin_fixed_div(x2, twin_int_to_fixed(3)),
y1_ratio + twin_fixed_div(y2, twin_int_to_fixed(3)), x2, y2);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m curious why the function
twin_path_quadratic_curve()
does not use the function_twin_matrix_x()
and_twin_matrix_y()
.
Yeah. I miss it, Thanks for the reminder.
Another question: Is there any difference compared to directly calculating the values x1, y1, x2, and y2, which are of type twin_fixed_t, and then calling twin_path_curve(path, x1, y1, x2, y2, x3, y3)?
It's because the p0
we get from _twin_path_current_spoint
is translated by the path->state.matrix
(I would like to call it the absolute coordinate) while (x1, y1)
, (x2, y2)
are not translated (call it the relative coordinate) and we should not mixed these two system coordinate in calculation.
Since we could't get the relative coordinate of p0
(it would require inverse matrix of path->state.matrix
or additional variable to store it), I calculate the point with absolute coordinate system and call _twin_path_scurve
to produce the curve.
I think the revised code would be like this?
void twin_path_quadratic_curve(twin_path_t *path,
twin_fixed_t x1,
twin_fixed_t y1,
twin_fixed_t x2,
twin_fixed_t y2)
{
twin_spoint_t p0 = _twin_path_current_spoint(path);
/* Convert quadratic to cubic control point */
twin_sfixed_t x1s = _twin_matrix_x(&path->state.matrix, x1, y1);
twin_sfixed_t y1s = _twin_matrix_y(&path->state.matrix, x1, y1);
twin_sfixed_t x2s = _twin_matrix_x(&path->state.matrix, x2, y2);
twin_sfixed_t y2s = _twin_matrix_y(&path->state.matrix, x2, y2);
/* CP1 = P0 + 2/3 * (P1 - P0) */
twin_sfixed_t dx1 = x1s - p0.x;
twin_sfixed_t dy1 = y1s - p0.y;
twin_sfixed_t cx1 = p0.x + twin_sfixed_mul(dx1, twin_double_to_sfixed(2.0/3.0));
twin_sfixed_t cy1 = p0.y + twin_sfixed_mul(dy1, twin_double_to_sfixed(2.0/3.0));
/* CP2 = P2 + 2/3 * (P1 - P2) */
twin_sfixed_t dx2 = x1s - x2s;
twin_sfixed_t dy2 = y1s - y2s;
twin_sfixed_t cx2 = x2s + twin_sfixed_mul(dx2, twin_double_to_sfixed(2.0/3.0));
twin_sfixed_t cy2 = y2s + twin_sfixed_mul(dy2, twin_double_to_sfixed(2.0/3.0));
_twin_path_scurve(path, cx1, cy1, cx2, cy2, x2s, y2s);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calculating (2/3)P1, (1/3)P0, and (1/3)P2 first would make it easier to read. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we could't get the relative coordinate of p0(it would require inverse matrix of path->state.matrix or additional variable to store it), I calculate the point with absolute coordinate system and call _twin_path_scurve to produce the curve.
However, why it need to get the relative coordinate of p0? My idea is calculating the cubic spline's P1 and P2 which is in the absolute coordinate system first and then using twin_path_curve()
to get the relative coordinate of p1, p2 and p3.
The function of twin_path_curve
is to change the point to relative coordinate and then change to twin_sfixed_t
.
The function of twin_path_quadratic_curve()
is to creat the cubic spline's P1 and P2 which is in the absolute coordinate system by P1 of quadratic spline and then use twin_path_curve()
is to change the point to relative coordinate and then change to twin_sfixed_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't get it well.
twin_path_curve
convert the input point with relative coordinate and convert it with _twin_matrix_x
and _twin_matrix_y
to point with absolute coordinate and pass it to _twin_path_scurve
and I just design twin_path_quadratic_curve
with same style( convert relative coordinate input point and pass it with absolute coordinate to _twin_path_scurve
)
Maybe you could simply write your idea with code again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void twin_path_quadratic_curve(twin_path_t *path,
twin_fixed_t x1,
twin_fixed_t y1,
twin_fixed_t x2,
twin_fixed_t y2)
{
twin_spoint_t p0 = _twin_path_current_spoint(path);
/* Calculate (2/3) * P1 first */
x1 -= twin_fixed_div(x1, twin_int_to_fixed(3));
y1 -= twin_fixed_div(y1, twin_int_to_fixed(3));
/* Reuse the function twin_path_curve() that already implements "converting
* relative coordinate input point and passing it with absolute coordinate
* to _twin_path_scurve()." */
return twin_path_curve(
path,
x1 + twin_fixed_div(twin_sfixed_to_fixed(p0.x), twin_int_to_fixed(3)),
y1 + twin_fixed_div(twin_sfixed_to_fixed(p0.y), twin_int_to_fixed(3)),
x1 + twin_fixed_div(x2, twin_int_to_fixed(3)),
y1 + twin_fixed_div(y2, twin_int_to_fixed(3)), x2, y2);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the p0 we get from _twin_path_current_spoint is translated by the path->state.matrix(I would like to call it the absolute coordinate) while (x1, y1), (x2, y2) are not translated (call it the relative coordinate) and we should not mixed these two system coordinate in calculation.
Just like I have mentioned, you couldn't calculate p0
( already translated to absolute coordinate by transition matrix) with x1
, x2
, y1
, y2
When I applied rotation (make the transition matrix is not identity) to the path
and use your code, the quadratic curve would be incorrect like :
That's why I said twin_sfixed_to_fixed(p0.x
) needs to be replaced with an operation involving the inverse matrix, making the calculation with x1 and x2 reasonable. But we don't want to use inverse matrix and we choose to calculate them in absolute coordinate system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
This is really interesting! By using cubic Bézier curves when only minor curve adjustments are needed, we can reduce the number of points required to store the font from three to just two. However, converting a cubic Bézier curve to a quadratic Bézier curve requires an approximation approach to make the two curves as similar as possible. |
bool sweep, | ||
twin_fixed_t radius_x, | ||
twin_fixed_t radius_y, | ||
twin_fixed_t cur_x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use
twin_point_t cur_point;
Replace
twin_fixed_t cur_x;
twin_fixed_t cur_y;
The advantage of doing this is that it directly separates the function of radius_x
from the reading the code here, clearly indicating that it is an object representing a point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have also considered replacing cur_x
and cur_y
with cur_point
. However, it might create an inconsistent style compared to other path functions like twin_path_arc
, twin_path_circle
, twin_path_ellipse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
I got it, close this review, please.
92e164e
to
86f3a23
Compare
28a3cfd
to
a3b3ebe
Compare
src/trig.c
Outdated
return TWIN_ANGLE_90; | ||
if (y == 0) | ||
return TWIN_ANGLE_0; | ||
twin_fixed_t current_x = x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use the variables current_x and current_y here? Can it just use the variables x and y instead?
apps/multi.c
Outdated
@@ -220,6 +220,52 @@ static void apps_jelly_start(twin_screen_t *screen, int x, int y, int w, int h) | |||
twin_window_show(window); | |||
} | |||
|
|||
static void draw_flower(twin_path_t *path, twin_fixed_t radius) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this code be modified to adjust dynamically, or is it unnecessary?
static void draw_flower(twin_path_t *path, twin_fixed_t radius, int number_of_petals)
{
const twin_angle_t angle_shift = TWIN_ANGLE_360 / number_of_petals;
const twin_angle_t angle_start = angle_shift / 2;
twin_fixed_t p_x = twin_fixed_mul(radius, twin_cos(-angle_start));
twin_fixed_t p_y = twin_fixed_mul(radius, twin_sin(-angle_start));
twin_path_move(path, p_x, p_y);
for (twin_angle_t a = angle_start; a <= TWIN_ANGLE_360; a += angle_shift) {
twin_fixed_t c_x = twin_fixed_mul(radius, twin_cos(a));
twin_fixed_t c_y = twin_fixed_mul(radius, twin_sin(a));
twin_fixed_t rx = radius;
twin_fixed_t ry = radius * 3;
twin_path_arc_ellipse(path, 1, 1, rx, ry, p_x, p_y, c_x, c_y,
a - angle_start);
p_x = c_x;
p_y = c_y;
}
twin_path_close(path);
}
src/trig.c
Outdated
/* First quadrant : angle | ||
Second quadrant : 180 - angle | ||
Third quadrant : 180 + angle | ||
Fourth quadrant : 360 - angle*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*
* First quadrant : angle
* Second quadrant : 180 - angle
* Third quadrant : 180 + angle
* Fourth quadrant : 360 - angle
*/
46e4e9d
to
18ce172
Compare
Add quadratic bezier curve support by converting quadratic spline to cubic spline. The conversion follows the formula: CP1 = P0 + 2/3 * (P1 - P0) CP2 = P2 + 2/3 * (P1 - P2) where: - P0, P1, P2 are quadratic control points - CP1, CP2 are the resulting cubic control points Ref: https://fontforge.org/docs/techref/bezier.html#converting-truetype-to-postscript
1c13b8f
to
13b8b78
Compare
src/trig.c
Outdated
} else { | ||
angle = TWIN_ANGLE_180 - twin_atan2_first_quadrant(y, -x); | ||
} | ||
return angle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it directly return without variable angle?
twin_fixed_t sign = (fa != fs) ? -1 : 1; | ||
double px_d = twin_fixed_to_double(px); | ||
double py_d = twin_fixed_to_double(py); | ||
double prx_d = twin_fixed_to_double(prx); | ||
double pry_d = twin_fixed_to_double(pry); | ||
|
||
twin_fixed_t A = | ||
twin_double_to_fixed(pry_d / (py_d + (pry_d / prx_d) * px_d)); | ||
twin_fixed_t B = | ||
twin_double_to_fixed(prx_d / (prx_d + (px_d / py_d) * pry_d)); | ||
twin_fixed_t C = | ||
twin_double_to_fixed(pry_d / (pry_d + (py_d / px_d) * prx_d)); | ||
|
||
twin_fixed_t pM = A - B - C; | ||
twin_fixed_t M = sign * twin_fixed_sqrt(pM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used double
here to calculate M
to avoid overflow. Should we introduce another fixed-point format to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it cannot use twin_fixed_mul
and twin_fixed_div
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am currently drawing a TinyVG image, and I found that the input value for an ellipse might be too large and could cause overflow (I have tried twin_dfixed
(Q23.8), but it would still overflow).
A example is
large_arc : 0 sweep : 1 rx 3423.000000 ry 3423.000000 cur 409.750000 28.250000 tar 401.375000 37.125000
comic
original (double)
overflow (Fixed point)
chart
ellipse parameter:
large_arc : 0 sweep : 1 rx 3423.000000 ry 3423.000000 cur 409.750000 28.250000 tar 401.375000 37.125000
original (double)
overflow (Fixed point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am currently drawing a TinyVG image, and I found that the input value for an ellipse might be too large and could cause overflow (I have tried twin_dfixed(Q23.8), but it would still overflow).
How about fixed-point type in Q31.32 format?
Type Name: twin_xfixed_t
where "x" could denote "extended precision"
Aspect | Q47.16 | Q31.32 |
---|---|---|
Range | ±140 × 10¹² | ±2.1 × 10⁹ |
Precision | Moderate (≈ 0.000015) | High (≈ 2.33 × 10⁻¹⁰) |
Implementation | Easier to manage scaling and rounding | Requires careful scaling and rounding |
src/trig.c
Outdated
return TWIN_ANGLE_90; | ||
if (y == 0) | ||
return TWIN_ANGLE_0; | ||
twin_angle_t angle = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
twin_angle_t angle = TWIN_ANGLE_0;
Use TWIN_ANGLE_0 for consistency.
Allow drawing elliptical arcs by specifying end points instead of center point, following some common vector graphic endpoint parameterization format. Ref: https://www.w3.org/TR/SVG/implnote.html
Add missing path functionalities required for rendering TinyVG (Tiny Vector Graphics) images. Specifically, the following features have been implemented:
Draws an ellipse segment between the current and the target point, where
radius_x
andradius_y
determine the both radii of the ellipse.large_arc
determine small(0) or larger(1) circle segment is drawn. Ifsweep
is 1, the ellipse segment will make a left turn, otherwise it will make a right turn.rotation
is the rotation of the ellipse.Draws an ellipse segment between the current and the target point, where
radius
determine the radius of the circle.large_arc
determine small(0) or larger(1) circle segment is drawn. Ifsweep
is 1, the circle segment will make a left turn, otherwise it will make a right turn.Draws a Bézier curve with a single control point.
Demo
2024-11-26.12.08.42.mov
Ref:
[1]:https://fontforge.org/docs/techref/bezier.html#converting-truetype-to-postscript
[2]:https://www.w3.org/TR/SVG/implnote.html