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

rounded rectangle #48

Merged
merged 2 commits into from
May 28, 2024
Merged

rounded rectangle #48

merged 2 commits into from
May 28, 2024

Conversation

juliapaci
Copy link
Contributor

related to #43

auto switching between rounded and non-rounded

how would we do this? seems polymorphic so rust traits would be the only option right?

@juliapaci
Copy link
Contributor Author

juliapaci commented May 27, 2024

or a simpler solution would be a condition in shape() i.e.

impl VelloVector for VelloRect {
    fn shape(&self) -> impl kurbo::Shape {
       if self.radius == 0 { kurbo::Rect::new(...)}
       else { kurbo::RoundedRect::new(...)}
    }
}

but im not sure which you had in mind

@nixon-voxell
Copy link
Member

yeah, I was thinking maybe just a simple if statement? haha not sure if that would worth the performance cost tho?

@juliapaci
Copy link
Contributor Author

juliapaci commented May 27, 2024

im currently struggling with object safety by return different types. Seems like a new Shape trait with object safety is necessary although it may lack features required for the object's manipulation.

As for performance, i think it would actually be much worse if we use return different types due to dynamic dispatch rather than if we just return a RoundedRect with a radius of 0 some of the time.

@nixon-voxell
Copy link
Member

Oh yes, of course, I have not thought of that! haha thanks for pointing it out, then I guess, the easiest solution would be the current implementation or add a new type VelloRoundedRect tho, I think the current implementation is more user friendly.

@juliapaci
Copy link
Contributor Author

I vote to keep the current implementation until linebender/kurbo#331 gets merged then rewrite with that in mind

@nixon-voxell
Copy link
Member

agreed, thanks for pointing out the PR as well :D

Copy link
Member

@nixon-voxell nixon-voxell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey juz a tiny change to fix the rust fmt error, thanks!

crates/bevy_vello_graphics/src/rect.rs Outdated Show resolved Hide resolved
@nixon-voxell nixon-voxell self-requested a review May 28, 2024 02:40
@nixon-voxell nixon-voxell merged commit c992078 into voxell-tech:main May 28, 2024
7 checks passed
@nixon-voxell
Copy link
Member

@aymey I also wanted to point out that your changes will be moved into the new repo I created: https://github.com/voxell-tech/bevy_vello_graphics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants