search

pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
849 stars 140 forks source link

`geometry` module #2267

Open itzpr3d4t0r opened 1 year ago

itzpr3d4t0r commented 1 year ago

I'm pleased to inform you that the pygame-geometry library is about to start to be officially ported to the pygame-ce repository. This process will be carried out in multiple phases, with the exact details still being determined.

The primary goal is to enhance the functionality of the library by introducing several new types of shapes, while retaining the familiar features found in pygame's Rect or Frect classes.

The main portion of the functionality consists of the following shape classes:

Each shape will include the following:

Please note that the current codebase may contain functionality that will not be included in the port or may undergo significant modifications.

If you are interested in exploring the complete codebase for the project, you can find it here:

Integration Progress

The integration will happen in phases, starting with Circle and later Line and Polygon. Each shape Will then be slowly built upon piece by piece.

Circle

Attributes

Methods

Line

Attributes

Methods

Polygon

Attributes

Methods

Rect & Frect

Methods

Other

zoldalma999 commented 1 year ago

Few questions I have related to the api, the implementation and how it will be moved to the main repo:

And finally... should we add this at all? I know you all worked on this hard, I do not want to discredit anyone... but you kind of just came here and opened an issue saying "we will do this" and a pr. As far as I know there was no recent discussion about it, not since creating the pygame-geometry repo, which was now a year(ish) ago. Not saying I want to 100% deny all this, but an earlier heads-up, more discussion and coordination would help a lot in making this more frictionless in my opinion.

And merging it into here would mean we have to support it no matter what. Possibly putting in new api specifically for new shapes, people maybe asking for more shapes for some reason, functions having another path for if a shape was passed in, etc... While if we don't merge it here, we could just make it a "strongly recommended" library. Kind of like sdl and all the other libraries do it (sdl_ttf, sdl_image, sdl_mixer, for example). They are maintained by the same people (for some/most of them), but are not part of the core.

But these are my 2 cents anyway.

itzpr3d4t0r commented 1 year ago

First of all thanks for commenting. Unfortunately for me I have this bad habit of opening PRs very fast with little notice, sorry for that.

Would I be able to use sprites with a different hitbox?

  • Setting a mask based on a polygon?
  • Would a draw.circle call now return a circle instead of a rect? Or how do we handle that? We can't just brake backwards compat, but since having circles it kind of feels weird to return a rect.
  • What about gfxdraw?
  • Can I get the bounding polygon of a surface, just like surf.get_bounding_rect?
  • This is not an "extensive list"... any other places where it would fit in that I missed?

These are all excellent ideas. Adding these shapes to pygame can enhance multiple functionalities while maintaining backward compatibility. I believe, however, that potential futher enhancements to existing functionalities should be addressed when those functionalities are actually proposed (with an Issue). The main goal of porting the pygame.geometry project is to introduce new classes, methods, and attributes, which have already been successfully implemented. So aside from stuff that we can't change because it would break back compat, there isn't much else to discuss about potential future additions in this specific moment in time.

Should we add this as a private module for the time being?

I'm neutral about this.

How to port over all the contributions to the main repo? I imagine you can't just move commits (but I am not that good with git, so maybe?). Is co-authoring and linking to the geometry repository enough? What are other options?

I'm not exactly sure about the process yet. For now I'm just taking code from the geometry project and trying to adapt it to the pygame repo without too much change. But yeah co-authoring will be much welcome. Since many people contributed to pygame-geometry i believe it's unlikely for every single person to open their separate PR to add their own changes, so there must be someone who will do that for them.

  • How do you plan to merge all of this here? Reviewing this amount of code is a lot of time, especially if we want more people to look through it.
  • "Please note that the current codebase may contain functionality that will not be included in the port or may undergo significant modifications." What does this mean? Isn't the linked implementation planned to be just "copied over"? Are you planning to change stuff while merging it into the codebase?

The plan is to slowly implement functionality, hopefully one shape at a time and with careful review and considerations. To address your review time concerns, the open PR is a foundation PR, so it necessarily has to be the most comprehensive and long one, all the other PRs can add any arbitrary functionality (even a single PR per method/attribute) separately, hopefully expediting review.

Are you planning to change stuff while merging it into the codebase?

Yes of course! Who thinks to copy something 100% from somewhere to an open source repo without changes?

Do we plan to do "F" versions? Or will it use floats when we release it? Maybe we could use python numbers?

The shapes all use double precision numbers to represent their attributes. We made this choice because it would be much more precise (especially after tons of transformations) and because python itself uses doubles by default, so it would be easier to implement since there would be less indirection.

And finally... should we add this at all? I know you all worked on this hard, I do not want to discredit anyone... but you kind of just came here and opened an issue saying "we will do this" and a pr. As far as I know there was no recent discussion about it, not since creating the pygame-geometry repo, which was now a year(ish) ago. Not saying I want to 100% deny all this, but an earlier heads-up, more discussion and coordination would help a lot in making this more frictionless in my opinion.

I'm not the one who is going to decide whether we should add this. But this story isn't new at all as you stated, we talked about this in the contributing chat months ago, we opened an issue on the old pygame repo (here: https://github.com/pygame/pygame/issues/3444) and it was much appreciated and everyone we talked to seemed enthusiastic about these additions. I opened the PR and went gun blazing partly because that's the only way to kickstart discussion and because it's my habit (a bad one but still a habit).

And merging it into here would mean we have to support it no matter what. Possibly putting in new api specifically for new shapes, people maybe asking for more shapes for some reason, functions having another path for if a shape was passed in, etc... While if we don't merge it here, we could just make it a "strongly recommended" library. Kind of like sdl and all the other libraries do it (sdl_ttf, sdl_image, sdl_mixer, for example). They are maintained by the same people (for some/most of them), but are not part of the core.

I personally don't like the idea of keeping this as a separate library. Mainly because this should be functionality that's easy to access for new users, especially because of the power that it has. Having it in a separate library means you have to download a separate package, and it also means that it should still be supported somehow, so someone has to do it anyway if we go down that path. Everyone i talked to seemed enthusiastic about these functionalities, so if we are scared to make it easily accessible to the widest amount of people we are just not doing the community's best.

AndreyVarvar commented 1 year ago

I really like this addition, and I myself was hoping that something similar will be implemented.

bilhox commented 1 year ago

I really like the idea of a new module, this will help us a lot. Two consequences however in my opinion of this new module :

Again, thank you very much for your hard work !

Starbuck5 commented 1 year ago

Would I be able to use sprites with a different hitbox?

Good idea. Right now the hitboxes are in a rect attribute. We could change to a hitbox attribute, or maybe rect could accept non rectangles.

@itzpr3d4t0r, I'd like to see a written out interface that you and the team plan on following for the shapes.

I'm interested to see what changes you would make to Rects to make it follow this interface. Like a generic collides method perhaps.

And as for the current PR, I'd like it to be a private module. Warning about compat at top, Circle not in default namespace, maybe even module hidden from docs, full 9 yards.

zoldalma999 commented 1 year ago

But yeah co-authoring will be much welcome. Since many people contributed to pygame-geometry i believe it's unlikely for every single person to open their separate PR to add their own changes, so there must be someone who will do that for them.

By co-athoring I mean the github supported way which should be done when someone makes a pr that adds someone else's work, even if it is a small change. I would even go as far as linking the section of the code added with blame on (like this for example), or the original commit that added it if it is a single commit (like this) (however, your idea is good too, let people who opened the pr on the geometry repo open it here as well). I want people to be properly credited, so I will try to be strict about this when reviewing, and I urge others to do the same as well.

The plan is to slowly implement functionality, hopefully one shape at a time and with careful review and considerations. To address your review time concerns, the open PR is a foundation PR, so it necessarily has to be the most comprehensive and long one, all the other PRs can add any arbitrary functionality (even a single PR per method/attribute) separately, hopefully expediting review.

That is good to hear. This also means then you are aware that it will take a lot of time as well, which is good. And since we are approaching it this way, making the module private and not advertising it would make more sense imo. Not until it is completely done.

Yes of course! Who thinks to copy something 100% from somewhere to an open source repo without changes?

I meant API changes, that are changed when opening the pr. Not after review, that are suggested. I am asking because then why not commit to the geometry module first and then copy them over? As I said before, I would like the geometry repo to be a place where one can see the proper credits of people, so this makes more sense in my eyes. This would also make merging into the pygame repo much faster, since we don't have to decide on the API itself, just the code.

I'm not the one who is going to decide whether we should add this. But this story isn't new at all as you stated, we talked about this in the contributing chat months ago, we opened an issue on the old pygame repo (here: pygame/pygame#3444) and it was much appreciated and everyone we talked to seemed enthusiastic about these additions.

Everyone i talked to seemed enthusiastic about these functionalities, so if we are scared to make it easily accessible to the widest amount of people we are just not doing the community's best.

Yes, I stated that it was discussed to some degree (again, a year ago now), but even that was not that thorough to be honest. Yes it is a start, but since then, you implemented the api. Where is the discussion about that? Because the only place I see it is in a thread in the contributing channel in discord with like 20 people in it. Did you ask the opinion of people (the everyone you are talking about here) about that? Were they still enthusiastic? Would they change add or remove anything? Saying "we are just not doing the community's best" while not asking them more in-depth about the very thing you are talking about sounds off to me.

This would also reduce the amount of API change that needs to be done while merging into pygame

I opened the PR and went gun blazing partly because that's the only way to kickstart discussion and because it's my habit (a bad one but still a habit).

This is not the only way to kickstart discussion, and is a bad attitude. Ask people to share their opinion, share easy to grasp and concrete plans and ideas, promote it. Yes, it is a way, but this is not the way it should be done. It just puts pressure on maintainers, adds another pr to the list that will not move for a long time and might misdirect people from caring about the planning to the code.

I personally don't like the idea of keeping this as a separate library. Mainly because this should be functionality that's easy to access for new users, especially because of the power that it has. Having it in a separate library means you have to download a separate package, and it also means that it should still be supported somehow, so someone has to do it anyway if we go down that path.

I mean, if someone can get pygame(-ce) then they can also get the extra geometry module without much effort. And while sure we would have to keep the geometry module alive, it would reduce the amount of maintenance the team has to do since we would not have to integrate all of geometry to other parts of the code.

\ And again I am not saying all of these because I so wildly dislike this module. I am saying all this because I want to see what our options are and how we should proceed with all this. I don't want to rush in and add a module only to realize that there is a better option that we can't go with at that point because it is too late. This is why I want more discussion, more planning, more concrete stuff. Which you seem to be against for some reason (this issue starts with "library is about to start to be officially ported", then open a pr immediately, not even once asking about opinions until someone actually does share it). All of my questions try to get these info out of you and see if there is a different or better way to do what you are trying to do.

Starbuck5 commented 1 year ago

I don't want to rush in and add a module only to realize that there is a better option that we can't go with at that point because it is too late

This is why this is going to be an experimental, hidden, unstable (all the good stuff) module at first.

Worst case scenario the momentum falls off and enters the zombie purgatory world of _sprite and _sdl2.audio.

@itzpr3d4t0r, I'd like to see a written out interface that you and the team plan on following for the shapes.

After pulling some teeth on discord I believe I saw an image of an API interface out on the pygame-geometry channel. If that exists, could you please post it here.

itzpr3d4t0r commented 1 year ago

This is the interface, we are still missing implementations for:

There are also a couple of uncertainties:

Class Collidable
Methods collidepoint, colliderect, collidecircle, collideline, collideswith, collideslist, collideslistall
Class Shape (Collidable)
Attributes center
Methods move, move_ip, update, copy, as_rect
Class Line (Shape)
Attributes a, b, xa, ya, xb, yb, length, angle, slope, centerx, centery
Methods flip, flip_ip, flip_ab, flip_ab_ip, as_circle, is_parallel, is_perpendicular, at, as_segments, as_points, scale, scale_ip, rotate, rotate_ip
Class Circle (Shape)
Attributes x, y, r, d, diameter, r_sqr, area, circumference, centerx, centery
Methods contains
Class Polygon (Shape)
Attributes vertices, verts_num, perimeter, area
Methods insert_vertex , remove_vertex , pop_vertex, is_convex, as_segments, flip, flip_ip, scale, scale_ip, rotate, rotate_ip
Geometry Module
raycast, multiraycast, regular_polygon, rect_to_polygon, is_line, is_circle, is_polygon
Starbuck5 commented 11 months ago

API suggestions looking this over

Bring contains up into the Shape interface? Since lines have 0 thickness lines probably can't contain anything, but Polygons can. Currently, Rect.contains is for when "argument is completely inside the Rect," maybe this will be difficult to implement in a generic shape <-> shape fashion?

Bring rotate up into the Shape interface? Rect.rotate could return a Polygon, Circle rotate would do nothing.