Head for vector graphics

[juliapaci]

migrated from voxell-tech/falcon#49

I feel like we could get away for this one without linebender/kurbo#331 if we are only using BezPath here

yeah, we can do that for now.

I was thinking it could simply be a component of the entity.

Oh yeah much better, i hadn't thought of this.

Resolves #1

[juliapaci]

@aymey There is also an alternative where we don't need to implement VelloVector for ArrowHead at all. We can resolve it in a bevy system when drawing the vello scene.

• Arrow shape (WIP) falcon#49 (comment)

When implementing custom build_* in the VelloVector trait for VelloArrow, im running into the same ish object safety issue:

e.g.

fn build_fill(&self, fill: &Fill, scene: &mut vello::Scene) {
    scene.fill(
        fill.style,
        default(),
        &fill.brush.value,
        Some(fill.brush.transform),
        &match self.head {
            ArrowHead::Triangle => kurbo::Triangle::new(...),
            ArrowHead::Square => kurbo::Rect::new(...),
            ArrowHead::Circle => kurbo::Circle::new(...),
        }
    );
}

the match expression should evaluate to a single type. So i feel like BezPath is a descent enough solution for now instead of static shape nonsense.

I was thinking it could simply be a component of the entity. Then when rendering we check for this component and perform the required computation to determine where to place the arrow head.

yeah this makes sense, however im unsure of a better way to do this other than updating every build_* implementation to account for the possible VelloArrow component via a query.

[nixon-voxell]

yeah this makes sense, however im unsure of a better way to do this other than updating every build_* implementation to account for the possible VelloArrow component via a query.

You could do it in a regular system, just like how shapes are being built into Scenes:

bevy_vello_graphics/src/lib.rs

Lines 93 to 122 in 539b2b3

	#[allow(clippy::type_complexity)]
	fn build_stroke_only_vector<Vector: VelloVector + Component>(
	mut q_vectors: Query<
	(&Vector, &Stroke, &mut VelloScene),
	(Without<Fill>, Or<(Changed<Vector>, Changed<Stroke>)>),
	>,
	) {
	for (vector, stroke, mut scene) in q_vectors.iter_mut() {
	*scene = VelloScene::default();
	// Build the vector to the VelloScene
	vector.build_stroke(stroke, &mut scene);
	}
	}
	#[allow(clippy::type_complexity)]
	fn build_fill_and_stroke_vector<Vector: VelloVector + Component>(
	mut q_vectors: Query<
	(&Vector, &Fill, &Stroke, &mut VelloScene),
	Or<(Changed<Vector>, Changed<Fill>, Changed<Stroke>)>,
	>,
	) {
	for (vector, fill, stroke, mut scene) in q_vectors.iter_mut() {
	*scene = VelloScene::default();
	// Build the vector to the VelloScene
	vector.build_fill(fill, &mut scene);
	vector.build_stroke(stroke, &mut scene);
	}
	}

Possibly something like this:

 #[allow(clippy::type_complexity)] 
 fn build_stroke_only_vector<Vector: VelloVector + Component>( 
     mut q_vectors: Query< 
-        (&Vector, &Stroke, &mut VelloScene),
+        (&Vector, &Stroke, Option<&ArrowHead>, &mut VelloScene),
-        (Without<Fill>, Or<(Changed<Vector>, Changed<Stroke>)>),
+        (Without<Fill>, Or<(Changed<Vector>, Changed<Stroke>, Changed<ArrowHead>)>),
     >, 
 ) {

[juliapaci]

Alright ill do that, thank you. although im worried that in the future this might become cluttered if we keep just adding parameters.

anyways, It seems like VelloArrow is still necessary as ArrowHead by itself cannot take inherit state like position and rotation from the parent/attached object. Also, having a VelloArrow type allows for customisation of offset, size, etc.

e.g.

#[allow(clippy::type_complexity)]
fn build_stroke_only_vector<Vector: VelloVector + Component>(
    mut q_vectors: Query<
        (&Vector, &Stroke, Option<&ArrowHead>, &mut VelloScene),
        (
            Without<Fill>,
            Or<(Changed<Vector>, Changed<Stroke>)>,
            Changed<ArrowHead>,
        ),
    >,
) {
    for (vector, stroke, arrow, mut scene) in q_vectors.iter_mut() {
        *scene = VelloScene::default();

        // Build the vector to the VelloScene
        vector.build_stroke(stroke, &mut scene);

        // Attach the possible arrow
        if let Some(arrow) = arrow {
            arrow.build_stroke(stroke, &mut scene);
        }
    }
}

here, arrow.build_stroke(...) cannot supply the position of vector's shape to cling on to. so its necessary for some intermediary VelloArrow for something like:

        // Attach the possible arrow
        if let Some(arrow) = arrow {
            vello_arrow.attach(vector.shape());
            vello_arrow.build_stroke(stroke, &mut scene);
        }

see 0038a11

[juliapaci]

Should we start doing docs just yet?

[juliapaci]

should we return self (with or without mutating it?) or just keep it as is? there are benefits to both but im not sure the kinda style you are looking for.

[nixon-voxell]

@aymey what do you think if we implement a new trait that specifies how arrows will be drawn for each trait:

pub trait VectorBorder {
    fn border_position(time: f32) -> Vec2;
    fn border_tangent(time: f32) -> Vec2;
    // etc.
}

Then we create a new system just for arrow:

fn build_stroke_only_arrow<Vector: VectorBorder + Component>(
    mut q_vectors: Query<
        (&ArrowHead, &Vector, &Stroke, &mut VelloScene),
        (Without<Fill>, Or<(Changed<ArrowHead>, Changed<Vector>, Changed<Stroke>)
    >,
)
// implement for fill only & fill + stroke

In these systems, utilize the VectorBorder trait to calculate how to create the required shape for the arrow and do NOT clear the scene (no to this ❌ *scene = VelloScene::default();)

To prevent cluttering, you can also implement these calculations as an impl function for ArrowHead:

impl ArrowHead {
    fn build_line<T: VectorBorder>(&self, vector: T) -> impl Shape {
        // ...
    }
    // implement for circle and line as well
}

And include all these into the build_vector function (not tested, some adjustment might be needed, but hope u get the idea):

pub(crate) fn build_vector<Vector: VelloVector + Component>() -> SystemConfigs { 
    ( 
        (build_fill_only_vector::<Vector>, build_fill_only_arrow::<Vector>).chain(),
        (build_stroke_only_vector::<Vector>, build_stroke_only_arrow::<Vector>).chain(),
        (build_fill_and_stroke_vector::<Vector>, build_fill_and_stroke_arrow::<Vector>).chain(),
    ) 
        .into_configs() 
 } 

Discussion

Optionally, I think if it's really hard to work with different shapes, we could split ArrowHead into TriArrowHead, CircArrowHead, and LineArrowHead.

[nixon-voxell]

I think as a first pass, we can just implement ArrowHead (or TriArrowHead if we chose that path) for line shape only. The rest can be added with follow up PRs.

[juliapaci]

In these systems, utilize the VectorBorder trait to calculate how to create the required shape for the arrow

if you look at VelloArrow::attach() we can derive all the necessary properties from just a Shape. But maybe sometimes they could be a little off.

I would rather it be dynamic but i understand the annoyance with it (like possibly wrong data and less elegant code). I do really like your VectorBorder trait idea if we go the static route.

Optionally, I think if it's really hard to work with different shapes, we could split ArrowHead into TriArrowHead, CircArrowHead, and LineArrowHead.

im slightly against this because i want the possibility in the future to add any shape (even a custom one the user creates) to be dynamically appended to another as a arrow head. Now that im writing this i realise that we could scrap ArrowHead and just take a Shape as the head for VelloArrow.

Im still happy to continue with your idea as you've described above, what do you think?

[nixon-voxell]

im slightly against this because i want the possibility in the future to add any shape (even a custom one the user creates) to be dynamically appended to another as a arrow head. Now that im writing this i realise that we could scrap ArrowHead and just take a Shape as the head for VelloArrow.

Yes, this idea has been floating in my mind for a while now since I wrote my last post hahaha. If that's the case, the functionality of this component will be to render at the border of any shape given an offset and a rotation_offset.

My Idea

We still reuse the BorderVector trait that exposes 2 functions: border_translation & border_tangent. These 2 values are responsible of figuring out where to place the shape that we want.

We introduce a new Component called Head:

#[derive(Component, ...)]
pub struct Head {
    pub shape_id: ShapeId,
    pub scale: f32,
    pub offset: f32,
    pub rotation_offset: f32,
}

Now, in order to know what to draw during the scene building phase, we introduce a new resource called Shapes that maps our ShapeId into a Vello Scene:

#[derive(Resource, ...)]
pub struct Shapes {
    pub scenes: HashMap<ShapeId, vello::Scene>,
}

pub struct ShapeId(Uuid);

Then, during the building phase, we can do something like this:

let translation = vector.border_translation(t);
let tangent = vector.border_tangent(t);
let rotation = // calculate rotation using tangent and rotation_offset

let transform = kurbo::Affine::default().with_translation(translation).then_rotate(rotation).then_scale(head.scale);

let head_scene = shapes.scenes.get(head.shape_id);
scene.append(head_scene, Some(transform));

[juliapaci]

Yes that's perfect! ill start implementing this tomorrow

[juliapaci]

Hey, sorry Ive been neglecting this pr for a while. Ive had a lot of school stuff to do but ill be fully back after my exams in like two weeks. For now, is the current changes what you had in mind? im a bit unsure especially with the "lib.rs" build_head changes.

[nixon-voxell]

I assume some parts of the code on VectorBoder trait impls are WIP so I leave them as is for now, I left a few suggestions as well, overall great stuff! Thanks as always!

[nixon-voxell]

btw you can also run cargo fmt or enable formatting on save on yr IDE to prevent CI/format failure XD.

[juliapaci]

btw you can also run cargo fmt or enable formatting on save on yr IDE to prevent CI/format failure XD.

ah yeah sorry i always forget.

[juliapaci]

should i revert 0062a9c?

[juliapaci]

83d9584 1a9ffc7 may be out of order but i think it should be fine. we can do better testing once we get a triangle shape or something

[nixon-voxell]

83d9584 1a9ffc7 may be out of order but i think it should be fine. we can do better testing once we get a triangle shape or something

We can create one triangle shape for this demo.

[juliapaci]

i think it would also look better if we lerp rotating around the vertices of the rect and bezpath, should i do this?

[nixon-voxell]

i think it would also look better if we lerp rotating around the vertices of the rect and bezpath, should i do this?

for rect it's alright, I think for bezpath, might wanna just leave it as is, some people might not close the path, so it's up to them to handle it I think...

[nixon-voxell]

not sure if you missed this but i think this is the last issue!: #3 (comment)

yes we should simplify, this is more of a POC hahaha so I decided to go with the easier to read route xd.

yeah, i think the more readable is nicer but if we ever end up changing it then we also need to change the match in here: 861dd7f

Doesn't the translation_offset and rotation_offset just add an additional offset to the final transform we get from border_translation and border_rotation? I will have a look at testing that in hello world example.

I think we will need to change that to the more optimized version anyway as no one is going to be reading the code from the user stand point hahahaha. I can get on doing that as well.

[juliapaci]

Doesn't the translation_offset and rotation_offset just add an additional offset to the final transform we get from border_translation and border_rotation?

it did until 0062a9c which we needed to do because we had no border impls for the shapes. Now that we have them i will undo it

[juliapaci]

I think we will need to change that to the more optimized version anyway as no one is going to be reading the code from the user stand point hahahaha. I can get on doing that as well.

i would still need to benchmark but it looks like it wont make a difference, if we expand the lerp()s we need to convert the points to Vec2s before we multiply them + with rust optimizations: factorizing the terms for d and e points are equivalent to expanding them and working with polynomials of degree 3.

e.g. for bezpath border_roatation:

            PathEl::CurveTo(p1, p2, p3) => {
                let current = current.to_vec2();
                let p1 = p1.to_vec2();
                let p2 = p2.to_vec2();
                let p3 = p3.to_vec2();

                let d = (1.0 - t) * (1.0 - t) * current + 2.0 * t * (1.0 - t) * p1 + t * t * p2;
                let e = (1.0 - t) * (1.0 - t) * p1 + 2.0 * t * (1.0 - t) * p2 + t * t * p3;

                (e.y - d.y).atan2(e.x - d.x)
            }

however, the lerp() implementation for kurbo::point::Point converts to Vec2 anyways (ig it goes back to a Point so it would be slower however we could just use the convert to Vec2 and use theVec2::lerp if we want to skip the useless Point creation):

    #[inline]
    pub fn lerp(self, other: Point, t: f64) -> Point {
        self.to_vec2().lerp(other.to_vec2(), t).to_point()
    }

and the kurbo::vec2::Vec2::lerp() that it calls simply does the same equation as we would if we manually optimized:

   /// Linearly interpolate between two vectors.
   #[inline]
   pub fn lerp(self, other: Vec2, t: f64) -> Vec2 {
       self + t * (other - self)
   }

and both of these are inlined so the calls will be optimized away anyways.

so overall i dont think there would be any benefit, although i still need to benchmark cause maybe the intermediate point variables of a, b, c etc. trick the compiler or something idk

[juliapaci]

also for rect (and bezpath but thats gonna be alot more difficult #3 (comment)) we probably need to keep a consistent speed but im not sure how

[nixon-voxell]

also for rect (and bezpath but thats gonna be alot more difficult #3 (comment)) we probably need to keep a consistent speed but im not sure how

That's a pretty hard and "ugly" problem to solve, I don't think it's in the scope of this PR. The way most people do is to chop the curve up into pieces of straight lines and estimate (yes you did not read wrong, estimate).

[nixon-voxell]

I think we will need to change that to the more optimized version anyway as no one is going to be reading the code from the user stand point hahahaha. I can get on doing that as well.

i would still need to benchmark but it looks like it wont make a difference, if we expand the lerp()s we need to convert the points to Vec2s before we multiply them + with rust optimizations: factorizing the terms for d and e points are equivalent to expanding them and working with polynomials of degree 3.

e.g. for bezpath border_roatation:

            PathEl::CurveTo(p1, p2, p3) => {
                let current = current.to_vec2();
                let p1 = p1.to_vec2();
                let p2 = p2.to_vec2();
                let p3 = p3.to_vec2();

                let d = (1.0 - t) * (1.0 - t) * current + 2.0 * t * (1.0 - t) * p1 + t * t * p2;
                let e = (1.0 - t) * (1.0 - t) * p1 + 2.0 * t * (1.0 - t) * p2 + t * t * p3;

                (e.y - d.y).atan2(e.x - d.x)
            }

however, the lerp() implementation for kurbo::point::Point converts to Vec2 anyways (ig it goes back to a Point so it would be slower however we could just use the convert to Vec2 and use theVec2::lerp if we want to skip the useless Point creation):

    #[inline]
    pub fn lerp(self, other: Point, t: f64) -> Point {
        self.to_vec2().lerp(other.to_vec2(), t).to_point()
    }

and the kurbo::vec2::Vec2::lerp() that it calls simply does the same equation as we would if we manually optimized:

   /// Linearly interpolate between two vectors.
   #[inline]
   pub fn lerp(self, other: Vec2, t: f64) -> Vec2 {
       self + t * (other - self)
   }

and both of these are inlined so the calls will be optimized away anyways.

so overall i dont think there would be any benefit, although i still need to benchmark cause maybe the intermediate point variables of a, b, c etc. trick the compiler or something idk

I see thanks for the exploration! I think we can look into optimizations / speed up in a separate PR/issue 🤔 . Let's push for feature completeness first I think haha.

[nixon-voxell]

I think in general most of the rough edges are catered for, right now all that's left is to check on missing docs and update the README 😄 ! Thanks for your awesome contribution @aymey !

[juliapaci]

No problem it was fun and you did most of the work anyways lol.

but whats going on with the example time looping for line and bezpath? shouldn't they continue going backwards after time > 1?

[nixon-voxell]

but whats going on with the example time looping for line and bezpath? shouldn't they continue going backwards after time > 1?

My reason was that line and bezpath are open ended shapes, so they should be allowed to go off shape?

[juliapaci]

yeah you are right it defiantly looks better and if the user wanted to loop around then they can manually adjust time for that.