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

Improve Shapes #121

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

Improve Shapes #121

wants to merge 9 commits into from

Conversation

ruffdd
Copy link
Member

@ruffdd ruffdd commented Oct 23, 2020

Lines can now be diagonal
add Circle

the Line shape can now go in all directions
@ruffdd ruffdd added enhancement New feature or request code provided code labels Oct 23, 2020
@ruffdd ruffdd self-assigned this Oct 23, 2020
@ruffdd ruffdd changed the title Add circle Shape Improve Shapes Oct 23, 2020
Copy link
Contributor

@buehlefs buehlefs left a comment

Choose a reason for hiding this comment

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

The new line implementation breaks rectangles. And for some lines the start and end coordinates are not part of the line (e.g. a line from 1,5 to 13,7).

@ruffdd ruffdd requested a review from buehlefs October 30, 2020 15:15
Copy link
Contributor

@buehlefs buehlefs left a comment

Choose a reason for hiding this comment

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

The line implementation should still be broken (see previous reviews)

@ruffdd ruffdd requested a review from buehlefs October 30, 2020 16:14
Copy link
Contributor

@buehlefs buehlefs left a comment

Choose a reason for hiding this comment

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

This fixes Rectangles but lines like (4,5)--(13,7) and (1,4)--(3,13) are off by one on one axis.

The error goes in the direction away from 0 of the axis.
The line (4,5)--(13,7) is drawn as (4,6)--(13,8) with a y error of +1
The line (4,-5)--(13,-7) is drawn as (4,-6)--(13,-8) with a y error of -1
If the coordinates are flipped (x := y and y := x) then the error is in the x axis direction

This could be a rounding problem.

Comment on lines +39 to +50
if (pos.getX() < x1) {
x1 = pos.getX();
}
if (pos.getX() > x2) {
x2 = pos.getX();
}
if (pos.getY() < y1) {
y1 = pos.getY();
}
if (pos.getY() > y2) {
y2 = pos.getY();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (pos.getX() < x1) {
x1 = pos.getX();
}
if (pos.getX() > x2) {
x2 = pos.getX();
}
if (pos.getY() < y1) {
y1 = pos.getY();
}
if (pos.getY() > y2) {
y2 = pos.getY();
}
x1 = Math.min(x1, pos.getX());
x2 = Math.max(x2, pos.getX());
y1 = Math.min(y1, pos.getY());
y2 = Math.min(y2, pos.getY());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code provided code enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants