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

Refactored the rotation transformation function and examples #64

Closed
wants to merge 14 commits into from

Conversation

nangahamandine
Copy link
Contributor

@nangahamandine nangahamandine commented Oct 24, 2023

  • I've updated the initial rotation transformation function to now normalize to a co-ordinate system with with the default center of (0, 0).
  • I also refactored the rotation transform function to use a conversion from Cartesian coordinates to polar coordinates, then rotate, then convert back to Cartesian coordinates. This therefore makes it simpler to understand.
  • I added a new example to test the ellipse rotation transformation and it works fine.
  • All rotation transformations are now displayed within the canvas
  • Finally, I added comments to make the implementations easier to understand.

These changes are based on the requirements of issue #60

Copy link
Collaborator

@nikochiko nikochiko left a comment

Choose a reason for hiding this comment

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

Nice work! @nangahamandine

I've added some comments.

Thanks for the additional comments throughout the code. Can you add them in another PR please and remove them from here? It's a cleaner commit history that way, and it also makes it easier to do better reviews.

examples/ellipse_rotate.ml Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
examples/rotate.ml Outdated Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
lib/shape.ml Outdated Show resolved Hide resolved
lib/shape.mli Outdated Show resolved Hide resolved
@nangahamandine
Copy link
Contributor Author

nangahamandine commented Oct 24, 2023

The screenshots based on the present changes can be seen below:

Screenshot (251)
Screenshot (250)

@nangahamandine
Copy link
Contributor Author

Nice work! @nangahamandine

I've added some comments.

Thanks for the additional comments throughout the code. Can you add them in another PR please and remove them from here? It's a cleaner commit history that way, and it also makes it easier to do better reviews.

Thank you. I'll take out the comments and update the code.

1 similar comment
@nangahamandine
Copy link
Contributor Author

Nice work! @nangahamandine

I've added some comments.

Thanks for the additional comments throughout the code. Can you add them in another PR please and remove them from here? It's a cleaner commit history that way, and it also makes it easier to do better reviews.

Thank you. I'll take out the comments and update the code.

@nangahamandine
Copy link
Contributor Author

nangahamandine commented Oct 25, 2023

I've updated the code based on all the requested changes made by @nikochiko

I encountered some issues with refactoring into a rotate_point function and when using pattern matching, hence the delay.
I succeeded to resolve all issues and everything works fine now.

Please review :)

@nangahamandine
Copy link
Contributor Author

These are the attached screenshots of the rotated shapes:

Screenshot (254)
Screenshot (255)

@nikochiko
Copy link
Collaborator

nikochiko commented Oct 27, 2023

I encountered some issues with refactoring into a rotate_point function and when using pattern matching, hence the delay.
I succeeded to resolve all issues and everything works fine now.

Awesome! I'm glad you were able to do it.

I added comments where I thought the changes weren't necessary.

At this point because our representation of rectangle / ellipse is not good enough, we aren't able to demonstrate rotation in examples properly. Once that is done we should be able to create more interesting examples!

@nangahamandine
Copy link
Contributor Author

nangahamandine commented Oct 27, 2023

I encountered some issues with refactoring into a rotate_point function and when using pattern matching, hence the delay.
I succeeded to resolve all issues and everything works fine now.

Awesome! I'm glad you were able to do it.

Thank you @nikochiko

I added comments where I thought the changes weren't necessary.

I'm not sure where, can you please reiterate so I could make the necessary changes.

At this point because our representation of rectangle / ellipse is not good enough, we aren't able to demonstrate rotation in examples properly. Once that is done we should be able to create more interesting examples!

@nikochiko nikochiko closed this Jan 19, 2024
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