Open nbroderick-code opened 1 year ago
Thanks for opening the ticket.
I'll have a look into these.
Some things, like __contains__
raising NotImplementedError is expected.
BezierCurve and Polygon requires individual points, rather than a list of points. Unpacking the list with a * operator will work:
points = [[100, 100], [50, 231], [170, 300], [400, 503]]
shape = shapes.BezierCurve(*points, batch=batch) # Fail, could not initialize.
Others look like potential bugs.
No problem. Thanks for looking into it.
And good call on *points. I guess I didn't look at the documentation close enough. Although...it is an unusual way to have to pass in a list of points. Would be nice if it were unpacked inside the shape class. But...I dunno. I could be wrong.
For the contains not being implemented in the shapes module, you're right that's expected. But I noticed that contains is avoided for all shapes that are made of just line segments. But, I was thinking a more intuitive behavior would be to see if the point fell on the line segment. Or maybe, more simply, if it just loops through all the vertices and sees if the point is one of them.
" if point in line: " or "if point in arc" Kind of makes sense that would be its behavior. But again, just a hobbyist giving my .02.
Thanks for all your work! I love Pyglet so much.
I made a commit here that ensures visible = False
will work on all sprites.
I also added the missing Line.width
setter.
It's a fair point about accepting a list of vertices, rather than individual. This would be a breaking change, if it was decided to do, so it would likely need to target pyglet 2.1.
It turns out that Line already has __contains__
defined. It's treated as a rotated rectangle (which is is, since it has width).
For other shapes, like BezierCurve, a series of "point on line" checks makes sense.
Arc is perhaps a little more ambiguous. A series of "point on line" checks makes sense, but so does treating it as a circle (Like we do for the Star).
For the BorderedRectangle
, rotation works OK as far as I can see. By default the rotation point is the bottom left corner, which is the default anchor point. This is the same as Sprites in pyglet.
Rotation for other shapes is simply not implemented. For something like a Line, it doesn't seem necessary. Someone is welcome to add it if they wanted to, however.
@nbroderick-code and @benmoran56 I, who add the following features, think add __contains__
to Arc and BezierCurve is not very difficult.
segments - 1
) and use the same way as Line.__contains__
for all segments to achieve this goal. It's extreme inefficient.@benmoran56 Thank you for fixing those things!
Responding to "For the [Rectangle and] BorderedRectangle, rotation works OK...by default the rotation point is the bottom left corner":
Star, Ellipse, and Sector do not rotate with the bottom left as their anchor point. Consistent anchor point for Shape rotation would be nice. Shapes rotating on their center by default has precedent outside of Pyglet too. Just look at any "professional shape manipulation" software (Photoshop, Illustrator, After Effects). "rotation anchor = center" is standard.
(I also happen to think sprites should rotate around their center by default, but that's a different can of worms.)
I will add a "feature request" for triangle and line rotation since you are right, that's not really a bug. Apologies for stuffing both bugs and feature requests in one ticket. I got a little loosey-goosey with what I considered a "bug", I guess.
The default rotation for objects is always around the origin point (which would be the first vertex point in OpenGL). This is why Sprites, and other objects rotate around the bottom left. Shapes like Circles and Stars have their origins at the center.
It would require a layer of abstraction to enforce a common rotation point, so we opt towards leaving it up to the user.
I'm working on a shader mock PR + an upcoming second attempt at #913 that should help prevent some of this in the future. In summary, making it easier to test inlined shared behavior should help us catch things like broken rotation in the future.
"rotation anchor = center" is standard.
I agree, but the question is whether it's worth adding.
It would require a layer of abstraction to enforce a common rotation point, so we opt towards leaving it up to the user.
It might be worthwhile to add this, or at least __contains__
/ collision abstractions from what I understand of reading #927 and #928.
Here's a hypothetical example from the Discord discussion around the latter:
clickable_concave_shape = PolygonCollider(
x=window.width// 2, y=window.height//2,
*list_of_points_with_concavity,
contains_point=raycast
)
if (mouse_x, mouse_Y) in concave_button_shape:
print("clicked!")
Describe the bug I tested all of the shapes in the shape class, many do not behave a way that the documentation implies they would. Here is a summary. There is a test code to help you see what I mean below.
System Information:
How To Reproduce I ran this test environment to find issues. I "Failed" shapes that did implement features the docs said they had. I also failed ones that did not have intuitive behavior.
Just uncomment one shape at a time to see the behavior.