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

Clickable arc issue#778 #1561

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

Conversation

estensland
Copy link
Contributor

Opening this for some feedback

For Issue #778

I have it working with most cases I have found. It took a between my schedule, the complexity, and how much of math I have forgotten.

I wanted it to work with samples/simple_arc.rb (if you put a click event on them) and it mostly does. Since those use nofill, but the default is not, I built out the code to handle both cases.

At opening of PR, I think the main code could use some more comments (similar to those added for #center_point). Also, the specs are just what I have been using to get coverage while I refactored. They could use some work to be clearer at what is being tested. However, I wanted to open this early because I am not sure where the code should live. Should it stay in arc.rb or be broken out somewhere else?

Besides just being a large chunk of logic within arc, it could apply to other shapes. Since at its core an arc is an oval, a good part of the methods would be useable in expansions to Oval#in_bounds?

# https://math.stackexchange.com/questions/76457/check-if-a-point-is-within-an-ellipse
# (x - h)**2 (y - k)**2
# ------- + ------- <= 1
# Rx ** 2 Ry ** 2
Copy link
Member

Choose a reason for hiding this comment

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

Love love love the reference!

end

def y_adjust_negative?(input_angle)
((input_angle >= 0) && (input_angle <= 1.5708)) || ((input_angle >= 4.71239) && (input_angle <= 6.28319))
Copy link
Member

Choose a reason for hiding this comment

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

Could probably benefit from some constants for these values

@jasonrclark
Copy link
Member

Hey @estensland! Sorry I've been out of touch. Had some eye problems that have cut back on after-hours coding.

This looks awesome, though! While I don't follow all the math perfectly, the way you've broken it up and named the methods I feel confident I could with a little time and reading.

I'll pull this down as soon as I have some time and test a few things out, but this looks well on its way to being ready.

@estensland
Copy link
Contributor Author

@jasonrclark All good I've been there too. It too a lot of iterations and various testing to get it like this...it is possible I didn't check a certain case so please let me know if anything looks off!

@estensland
Copy link
Contributor Author

@jasonrclark I noticed that I had not pushed up everything, I had added a bit more comments. If they are too detailed or what not let me know. I added using constants to them off your comment.

@PragTob PragTob changed the base branch from master to main July 5, 2020 10:58
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