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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add parabola #1016

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add parabola #1016

wants to merge 1 commit into from

Conversation

itsCryne
Copy link

This would resolve #982

There may still be some rough edges as this is really my first time contributing to a bigger GUI project 馃槆

Most code of the parabola is based upon the existing rectangle and quadratic bezier shapes.

@flxzt
Copy link
Owner

flxzt commented Feb 22, 2024

Thanks! The code itself looks good, but I don't think we need to introduce a dedicated Parabola Shape. We could use the already existing geometric primitives and the parabola builder could just emit a quadratic bezier curve.

@Doublonmousse
Copy link
Contributor

Wouldn't that (introduce a new shape) be a way to future-proof the implementation if we ever decide that we want to add better editing capacities (like editing the anchor points after the shape is constructed) in the future that are shape-specific ?

Right now this doesn't matter, but it could in the future

@flxzt
Copy link
Owner

flxzt commented Feb 22, 2024

I don't really see the benefit of that. If you want to edit the shape later you could also implement it on the bezier curve, like being able to move the control points. I also want to keep the number of primitives low, otherwise we end up with a new shape for every mathematical function..

Edit: and you can already do that, the parabolic shape is retained when resizing the curve as a selection

@itsCryne
Copy link
Author

itsCryne commented Apr 3, 2024

Sorry for not communicating for so long - I got busy as the summer term started.
I'll try to find some time to change the implementation as suggested above in the next few weeks.

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

Successfully merging this pull request may close these issues.

Shapes: Add a Parabola
3 participants