A huge change of pyglet.shapes

[zhengxyz123]

In this PR, I made the following changes:

1. Create pyglet.shapes2d
2. Add a new Catenary shape
3. Move pyglet.shapes to pyglet.shapes2d.drawable
4. Add pyglet.shapes2d.collidable, a module focus on 2D collision detection
5. Add an example(collision.py)
6. Some other changes

Besides, type hints are added to pyglet.shapes2d, and #857 has done the same thing.

In addition, I tried to use sphinx-recommended way to write the comments in pyglet.shapes2d:

:param int foo: bar

[Ross-TheBoss]

Here are my thoughts:

There is quite a lot of duplicated code as a result of this design.

• The ShapeBase has all the attributes/functions of the CollisionShapeBase, and some additional attributes/functions.
• The CollisionCircle, CollisionEllipse, CollisionRectangle and CollisionPolygon have the same properties as their shape counterparts.

I imagine those using the pyglet.shapes, may want to add collision detections to a drawable shape, and this would require having both a drawable shape and collidable shape. You would have to make sure any changes you make to the positioning/rotation of the drawn shape is also performed on the collidable shape.

Additionally, not all shapes which once had collision detection (by doing point in Shape), have a collision detection class (e.g., Sector, Line, BorderedRectangle, Triangle and Star).

I suggest instead of having colliable shapes classes, you make them functions. For example, you can have a point_in_circle(x, y, radius, point, rotation=0, anchor_x=0, anchor_y=0) function, a point_in_ellipse function and a point_in_rectangle function. These functions can be used by the drawable shape classes (via the contains method), but also by the user in case they want to perform collision detection, without having to draw a shape.

[zhengxyz123]

@Ross-TheBoss collision detection isn't mean point in shape entirely. They should also provide something like:

• Is part of a circle in a rectangle?
• Is part of a rectangle in a combination of circles?
• etc.

which are very difficult to implement and will be implemented later.

Only considering circle, ellipse and rectangle, we will have 6 different ways to test whether they are collided or not.

We also should confirm that a in b and b in a return a same value.

So in the above condition, using function isn't a wise decision.

A CollisionShape is very useful when it comes across a Sprite(not a Shape).

---

Additionally, not all shapes which once had collision detection.

• Sector: I imagined that it would be very complicated to realise collisions between shapes for it afterwards and pretended to claim that I had forgotten.
• Line: At the time, I was writing the code for point in Line as a different shape than a Rectangle to get better efficiency, but a Line is really a Rectangle.
• BorderedRectangle: Just Rectangle.
• Triangle is a polygon.
• Star: too difficult.

[pushfoo]

I'll try to post full comments on the code later today.

Re: Ross-TheBoss

There is quite a lot of duplicated code as a result of this design.

To some degree, this is unavoidable for any performance-optimized Python code. Inlining helps avoid the high cost of function calls and . lookups in Python.

The CollisionCircle, CollisionEllipse, CollisionRectangle and CollisionPolygon have the same properties as their shape counterparts.

This could be a good choice, see my next response:

You would have to make sure any changes you make to the positioning/rotation of the drawn shape is also performed on the collidable shape.

For the already-existing shapes or anything that's vertex-based, this is actually simple: we don't have to update anything because we can delegate vertex_list.position and properties to an underlying shape object. SAT should work with this for points.

Re: PR / zhengxyz123's comment

Some of this PR is very good, but may need to be split up like #/927 was. I'll try to post more comments on that in the next few hours. In the meantime, here's a recap of what I posted on Discord:

The fundamental question: Is it worth it to implement multiple collider types if we can get away with using polygons?

Remember:

1. pyglet leans toward being an OpenGL, input, and media abstraction layer rather than a full game engine
2. Physics engines and other binary-backed modules will probably do a better and faster job at handling shape collisions

which are very difficult to implement and will be implemented later.
Sector: I imagined that it would be very complicated to realise collisions between shapes for it afterwards and pretended to claim that I had forgotten.

Composable colliders could address this:

# This is like any(), but for colliders
sector_collider = CompositeCollider(
  PolygonCollider(*triangle_params),
  CircleCollider(*circle_params)
)

However, this raises other questions:

1. Should we sort to place smallest first?
2. How would we define smallest? area? width? height?
3. How do we handle edge cases?
   1. Huge width / height, tiny area
   2. How do we handle sorting if a reference to an internal object is could be kept elsewhere and used to change it?

The easiest answer is to make the implementation limited and only support polygons.

[zhengxyz123]

@pushfoo I don't entirely argee with you. The most important reason is that using polygons is too inefficient.

Here is my solution: We(pyglet) just implement collision between polygons, but leave users a way to allow them to access more efficient algorithms(the CollisionShapeBase.is_collide method).

[shenjackyuanjie]

here is what I think.

• Is good to have ways to detect collision

• Also some simple shape collision is also verry useful and powerful in some UI usage.

• Too complex collision detect functionality will make the code too complex and some times with low performance.

So in a actual situation, if I need some simple UI collision detect(btw, why would you need that?), I do will use this code
But if there is some really complex shape or too many of them, I will chose to use some physic engine to deal with it, such as this (in rapier)

[zhengxyz123]

Well, there are some situations and solutions:

1. Alex just wants to render something, he do not need collision system.
2. Ben wants to know whether a point is inside a shape, he could use (x, y) in Circle, which is provided by CollisionCircle.
3. Carel is writing a pong in pure pyglet for demonstration purpose, he could use the collision system and add some action when ball is collided with bats and walls.
4. Dan adds a lot of CollisionRectangles and wants collide them with each other, he can use a CollisionShapeManager(which should be implemented by himself for specific application).
5. shenjackyuanjie wants to write a game with so many collision objects and with high performance, a physical engine like pymunk could be used. Or he could use a game engine that is more tightly integrated with the physics engine, which would be easier.

So, for situation (3) and (4), a built-in collision system is useful.

And for situation (5), create inheritances of Collision* which work with physical engine well is a good idea. It's same as you want a BorderedRectangle with different color on border and inner content.

If you read DESIGN for how pyglet should be, a collision system was suggested about two decades ago. But if this collision system needs to be connected to a physics engine in order to work, instead of providing some simple function, a part of the users will not accept it. And those who want to use it in a more complex and efficient application through a physics engine, we can also make an example to show them how they should start.

In summary, there is nothing wrong with adding a collision system, and one can choose to use it directly or rewrite it for better performance.

[Ross-TheBoss]

I now understand why the drawable and collision shapes should be seperate. But now I'm wandering if they are so seperate that the collision detection shouldn't be included within the shapes2d module. Collision detection for a shape is not neccessarily exclusively used by the drawn shape. For example, the CollisionRectangle could used with UI elements (such as buttons) and Sprites or even different shapes as an approximation for efficiency.

In the design, it is suggested that collision detection is part of the mathematical euclid module. I suggest having the modules:

• collision.py for efficient collision detection between simple 2d (or even 3d) shapes.
• shapes.py for drawing 2d shapes, this could use the collision module to add some collision detection convience methods.
• model.py for drawing 3d models/shapes

[zhengxyz123]

It's also interesting that you think differently than I do.

But essentially, they are both 2D shapes, aren't they? It's just that one(specialise in drawing) is visible and the other one(specialise in collision detection) is not.

You should also be aware that DESIGN was written about two decades ago, and the current module structure is different from what it describes.

Differ from you, I suggest having the following modules:

• shapes2d
• shapes3d
  • drawable.py: cubes, balls, etc.
  • collide.py: for 3D collision.
• model.py: for drawing 3D models(it is not a shape).

3D is entirely different from 2D, especially in rendering and collision detecting(it's more complex than the 2D one). We should use a unique way to treat it, or your collision.py will be confusing.

This PR will be merged in the development branch for future feature. So we can discuss a suitable structure.

[pushfoo]

I apologize for the delay in reviewing this. Large PRs like this one make are hard to review properly. It's easier to review and merge them when they arrive in smaller chunks.

In short:

1. Thank you for adding docstrings! I've provided instructions for a converter script below to help them match the project's preferred format.
2. Like other reviewers, I also have concerns about the collision & shapes design which are outlined in the review comments and Enhancement: Read-only vertices property for shapes & sprites #939
3. There some more minor code quality and organization concerns which are easier to address

I suggest doing the following while the collision design continues to be discussed:

1. Split the changes to customtypes.py into a new PR. These should be quick to merge.
2. To save you the trouble of converting from rst format to the project's preferred Google style, try the following:
   1. pip install docconvert
   2. Put this in doc_convert.json:

      {                                                                                                       
          "input_style": "rest",
          "output_style": "google",
          "accepted_shebangs": [
              "python"
          ],  
          "output": {
              "first_line": true,
              "remove_type_backticks": "false",
              "use_types": false
          }   
      }

   3. Run this for each path/to/file_using_rst.py:

      docconvert --config --in-place doc_convert.json path/to/file_using_rst.py

[pushfoo]

I do not think this should be deleted. This module's contents are still in use around the codebase, and the doc page for it still builds: https://pyglet.readthedocs.io/en/latest/modules/canvas.html#pyglet.canvas.Canvas

[pushfoo]

The changes to this file should be a smaller, separate PR since they're useful in general. I will also have further suggestions for the type annotations in that PR.

[pushfoo]

How often does this need to run? I haven't benchmarked this, but the repeated use of isinstance is going to make it slow.

[pushfoo]

I know keeping the entire text of the license annoying, but this is the easiest way to meet legal requirements for most of the world.

Suggested change

	# Copy from https://github.com/wsilva32/poly_decomp.property
	# License under MIT.
	"""Polygon decomposition helpers
	Copied from https://github.com/wsilva32/poly_decomp.py
	MIT License
	Copyright (c) 2016 Will Silva
	Permission is hereby granted, free of charge, to any person obtaining a copy
	of this software and associated documentation files (the "Software"), to deal
	in the Software without restriction, including without limitation the rights
	to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
	copies of the Software, and to permit persons to whom the Software is
	furnished to do so, subject to the following conditions:
	The above copyright notice and this permission notice shall be included in all
	copies or substantial portions of the Software.
	THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
	IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
	FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
	AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
	LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
	OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
	SOFTWARE.
	"""

[pushfoo]

Although this particular file may work, I'm concerned about its efficiency.

Can we explore caching results and other optimizations in future revisions?

[shenjackyuanjie]

how about make it like
Number = Union[int, float]
and
RGB = Tuple[int, int, int]
RGBA = Tuple[int, int, int, int]
Color = Union[RGB, RGBA]

[pushfoo]

I think this is in response to the wrong comment / file, but I generally agree. I have further changes to suggest as well, but the types changes should really be their own PR.

[pushfoo]

Suggested change

	"""An algorithm to detect whether two polygons intersect or not.."""
	"""Return whether two polygons intersect or not.
	Args:
	polygon1: The first polygon to test
	polygon2: The second polygon to test
	Returns:
	Whether the two polygons intersect
	"""

[pushfoo]

As I mentioned in the comments on drawable.py, I think drawables shouldn't be obligated to rely on colliders.

If we do so, we may be able to significantly simplify the design here.

[pushfoo]

My instinct is there's a better general approach, especially since we'll have ugly method names. For the immediate future, we should try to make full use of these:

1. The := operator (3.8+)
2. the default value argument for getattr

I'm not sure if encapsulating this as a function is worth it, so maybe something like this could be cleaner?

Suggested change

	method = f"collide_with_{other.__class__.__name__}"
	if hasattr(self, method) and callable(getattr(self, method)):
	return getattr(self, method)(other)
	method = f"collide_with_{self.__class__.__name__}"
	if hasattr(other, method) and callable(getattr(other, method)):
	return getattr(other, method)(self)
	if hasattr(self, "get_polygon") and hasattr(other, "get_polygon"):
	self_method_name = f"collide_with_{other.__class__.__name__}"
	if callable(self_method := getattr(self, self_method_name, None)):
	return self_method(other)
	other_method_name = f"collide_with_{self.__class__.__name__}"
	if callable(other_method := getattr(other, other_method_name, None)):
	return other_method(self)
	try:

[pushfoo]

continuing from line 68

Suggested change

	raise TypeError(
	"No collision detection method found between "
	f"{self.__class__.__name__} and {other.__class__.__name__}"
	)
	except AttributeError as e:
	raise TypeError(
	"No collision detection method found between "
	f"{self.__class__.__name__} and {other.__class__.__name__}"
	) from e

[pushfoo]

For each of these shapes, we seem to be iterating over the points 3 times:

1. when the vertices are initially created for the drawable shape
2. when we call get_polygon here
3. when we copy them to the polygon instance

It would be easier to calculate vertex positions once and reference them as outlined in #939. If nothing looks wrong with that PR, I'll try to implement it in the coming days.

[pushfoo]

I'm still not sure of the current design for TypeA x TypeB specialized collision. I'm going to keep thinking about that in the coming days.

But now I'm wandering if they are so seperate that the collision detection shouldn't be included within the shapes2d module.

I could go either way on this. With the changes proposed in #939, putting them elsewhere could make sense since anything with a vertices property returning Sequence[Point2D] a can collide. This would not only be collider objects, but also sprites, drawable shapes, and custom classes.

It also allows doing other neat things. For example, you could do the following in a prototype without needing shaders:

1. Use a 3D shape or model as a player character, puzzle element, or both
2. Project its outline as 2D group of vertices
3. Perform 2D collision against sprites or other 2D shapes

It wouldn't be performant, but it would let you validate gameplay ideas quickly.

[zhengxyz123]

@pushfoo Thanks for your advice.

First, I agree with spliting the changes to customtypes.py into a new PR, but I'm not familiar with this. I need your help.

The algorithm implemented in poly_decomp.py will not be used unless a concave polygon is encountered, which is very rare. It's also very complex and I don't know how to optimise it.

I found that this algorithm just convert javascript to python without using python features.

Here is a neat way to build collidable shapes from drawable shapes:

CollisionPolygon.from_shape(...)
CollisionCircle.from_sprite(..., radius, anchor_position, rotation)

If we want to update data when the sprite changes it's position, more code should be added.

[zhengxyz123]

To save you the trouble of converting from rst format to the project's preferred Google style.

It seems that the rest of pyglet uses a different format and the docconvert wasn't work on some methods like Arc.__init__, BezierCurve.__init__ and Catenary.__init__.

The above was solved by removing _draw_mode=....

[zhengxyz123]

Conclusion before revision, polygonSlice wasn't used in the polygonQuickDecomp, which is used by pyglet to convert concave polygons to several convex polygons.

[zhengxyz123]

@pushfoo I have changed a lot of things based on your review.

But I'm confusing on this.

[pushfoo]

Thank you! I'll try to take a closer look at the changes in the next 2 days.

But I'm confusing on this.

The idea is to eliminate the calls to hasattr by replacing this line with a try::

        if hasattr(self, "get_polygon") and hasattr(other, "get_polygon"):

An AttributeError will be raised if either method is missing anyway, so there's no point in checking if they're there beforehand. Catching the AttributeError achieves the same thing.

The from e at the end attaches the original AttributeError info. This isn't strictly necessary, but it helps preserve the exact point the issue occurred at while still presenting the type you selected as more meaningful.

To my understanding, the speed boost from eliminating function calls is greatest for versions earlier than 3.11. The current design of this makes it harder to measure the impact of removing the function calls due to the GL context involved, but the old versions still have years of support left ahead of them. It's probably worth the odd looking syntax here.