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

Issue 9 Possible Fix (Box-to-Self implementation) #13

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

Conversation

olkivan
Copy link

@olkivan olkivan commented Oct 14, 2020

Hi, Steve, could you please take a look at my fix for the Box-to-itself case? Here is an algorithm and a sample component which utilizes the algo. Could you please provide your opinion, might it solve the problem? I haven't provided the test cases yet, until you confirm it's the right way to go. I'd appreciate any feedback. Thanks!

@steveruizok
Copy link
Owner

Hey, I'll take a look at this over the weekend!

@olkivan
Copy link
Author

olkivan commented Oct 17, 2020

Great! Looking forward to your feedback!

@steveruizok
Copy link
Owner

Hey Ivan. Thanks for the PR, I've taken a look at it now. I'd like to bring its API into line with the other functions in the library (which are unusual, I know).

I'm also considering applying some of what I learned while working on this project, both to this problem at to some of the other arrows, too. That would be a bigger change though—moving from quadratic curves to ellipse segments, as you're doing here—and might be better for a 1.0.0. release.

So I need to make some decisions before I can get it ready to merge. Until then, your implementation is pretty well isolated so you should be able to use it on the side if it's an urgent need for a project. Thanks again :)

@olkivan
Copy link
Author

olkivan commented Oct 20, 2020

Steve, maybe it's a good idea to have both options - to use quadratic curves or to use ellipse segments (if it even possible). Anyway thanks for the feedback and I'll be waiting for the 1.0.0 release :)

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.

2 participants