Skip to content
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

gpui: Add move_to method to Path #22808

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

huacnlee
Copy link
Contributor

@huacnlee huacnlee commented Jan 8, 2025

Release Notes:

  • N/A

Continue #20499

We need this to draw Path. Before this change, we only have line_to, but it is not enough.

Show case

To draw a stroke line is not just to line_to, there need to consider the angle to get shift to draw next point. GPUI have not yet provided this things, but the 3rd-library are have.

For example use tiny-skia:

cargo run -p gpui --example painting
image

Before version:

image

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 8, 2025
@huacnlee huacnlee changed the title gpui: Add move_to method to Path. gpui: Add move_to method to Path Jan 8, 2025
@huacnlee
Copy link
Contributor Author

huacnlee commented Jan 8, 2025

Also see JakkuSakura/plotters-gpui#2

image

Previously, GPUI's Path did not have some algorithms for stroke, so we needed to do some complex calculations ourselves. As shown in the figure above, the calculations were actually wrong, and the lines in some directions were not drawn correctly.

For example the plotters-gpui do stroke like by:

https://github.com/JakkuSakura/plotters-gpui/blob/ac70ae70889e8a5f9c7880e826243a23c232c74e/src/line.rs#L55-L76

let (Some(first), Some(last)) = (self.points.first().copied(), self.points.last().copied())
else {
    return;
};

let mut angle = f32::atan2(first.y.0 - last.y.0, first.x.0 - last.x.0);
angle += std::f32::consts::FRAC_PI_2;
let shift = point(width * f32::cos(angle), width * f32::sin(angle));

let mut reversed_points = vec![first + shift];
let mut path = Path::new(first);
for &p in &self.points[1..] {
    path.line_to(p);
    reversed_points.push(p + shift);
}

// now do the reverse to close the path
for p in reversed_points.into_iter().rev() {
    path.line_to(p);
}

cx.paint_path(path, self.color);

If we can introduce something like tiny-skia, relying on the algorithms they provide, we can simplify this process.

The new version by tiny-skia:

image

Of course, this is actually two things. First, GPUI's Path lacks move_to, which can easily correspond to other vector graphics, such as SVG Path.

cc @JakkuSakura

@huacnlee
Copy link
Contributor Author

huacnlee commented Jan 8, 2025

After this merged, the next thing we can push to enable anti-aliasing.

Recently Blade have this support: kvark/blade#213, cc @kvark

@JakkuSakura
Copy link

Thanks for pointing out. The algorithm was written so for lacking such stroking algo and for performance consideration

@Angelk90
Copy link
Contributor

Angelk90 commented Jan 8, 2025

@huacnlee and @JakkuSakura : If we wanted to animate a graph showing the performance of a program, how much of an impact would it have on the RAM usage? Would the animation require a significant amount of resources, and could it impact the overall performance of the system?

Registrazione.schermo.2025-01-08.alle.09.33.56.mov

@huacnlee
Copy link
Contributor Author

huacnlee commented Jan 8, 2025

@huacnlee and @JakkuSakura : If we wanted to animate a graph showing the performance of a program, how much of an impact would it have on the RAM usage? Would the animation require a significant amount of resources, and could it impact the overall performance of the system?

Registrazione.schermo.2025-01-08.alle.09.33.56.mov

NO! Animate is not have extract RAM, at least not a very noticeable difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants