Skip to content

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.

4 participants