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

Shape: add exact bounding box calculation and add align method #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Funami580
Copy link
Contributor

I hope I got it all right. What do you think?

@CoffeeStraw
Copy link
Owner

Hey there! Thank you for the PR, much appreciated.

I will need to look into details of the logic behind your implementation (I see that you also linked stackoverflow). But for now, here's some thoughts: what about not having a separated method, but just move bounding_exact into bounding and introduce a new parameter like exact? Moreover, we will need to update docstring by clarifying that the exact method is more expensive than the standard one, so it should be used only when needed.

And finally, it is pretty confusing to introduce the "an" parameter in the move method. E.g. what does it mean to move a shape from position (0,0) to position (100,20) with an=3 or an=6? Alignment for subtitles generally affects both starting position and final position, so it is really strange... Indeed, you only use alignment when you want to "align to center" your shape... At this point, I think it is better to move the logic you've implemented to a separated method, like "Shape.center".

@Funami580 Funami580 force-pushed the bounding_exact branch 2 times, most recently from 9df0e35 to 0e8eafe Compare February 28, 2022 19:06
@Funami580 Funami580 changed the title Shape: add bounding_exact method and improve move method Shape: add exact bounding box calculation and add align method Feb 28, 2022
@Funami580 Funami580 force-pushed the bounding_exact branch 4 times, most recently from 0453fc9 to 959bfed Compare March 1, 2022 17:26
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