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

geometry module, Circle base #2268

Merged
merged 14 commits into from
Oct 8, 2023

Conversation

itzpr3d4t0r
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r commented Jun 22, 2023

Follows #2267.
This PR aims to start the port with the geometry module base and a basic Circle implementation, comprehensive of (see credits below):

  • Circle object instantiation
  • Circle x, y, r attributes
  • copy() function
  • repr/str functionality

The PR also adds two new functions for getting double values from python objects that are (see credits below):

  • pg_DoubleFromObj
  • pg_TwoDoublesFromObj

The addition of these two functions is critical as all shapes will be using doubles as values.

typestubs, docs and tests were also added (see credits below).

Note: Extra changes to adapt to pygame-ce's codebase are all authored by me. I also took the liberty to remove a couple of old/meaningless C comments that are still there in the original codebase, rename a macro originally named by @Emc2356 to make it clearer and slightly modified docs sections since they wouldn't work with sphinx rules anyway (docs all authored by me already)

The module will be in experimental mode for quite a while, and docs reflect that:
image
You can search for geometry and find it but it won't appear anywhere on the main page.

Credits

Geometry Project:
For code, docs and tests:
@novialriptide @Emc2356 @itzpr3d4t0r @ScriptLineStudios @avaxar @Matiiss @newpaxonian @maqa41 @blankRiot96
Also thanks to @Starbuck5 for kickstarting the idea and the occasional help!

Functionality added in this PR

@itzpr3d4t0r itzpr3d4t0r added the New API This pull request may need extra debate as it adds a new class or function to pygame label Jun 22, 2023
@itzpr3d4t0r itzpr3d4t0r changed the title geometry module base + Circle base First implementation of geometry module, Circle Jun 22, 2023
@itzpr3d4t0r itzpr3d4t0r changed the title First implementation of geometry module, Circle First implementation of the geometry module, Circle Jun 23, 2023
@itzpr3d4t0r itzpr3d4t0r marked this pull request as ready for review June 23, 2023 17:08
@itzpr3d4t0r itzpr3d4t0r requested a review from a team as a code owner June 23, 2023 17:08
@itzpr3d4t0r itzpr3d4t0r added the geometry pygame.geometry label Jun 23, 2023
@itzpr3d4t0r itzpr3d4t0r changed the title First implementation of the geometry module, Circle geometry module, Circle base Jun 23, 2023
src_c/geometry.c Outdated Show resolved Hide resolved
test/circle_test.py Outdated Show resolved Hide resolved
Comment on lines 182 to 184
:doc:`ref/geometry`
Geometry types and operations.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think should be added so prominently to the mainpage, at least right now.

Copy link
Member

Choose a reason for hiding this comment

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

This is not resolved.

_pg_circle_set_radius(PyObject *value, pgCircleBase *circle)
{
double radius = 0;
if (!pg_DoubleFromObj(value, &radius) || radius <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

If radius is less than 0 wouldn't this return error without setting an error?

Copy link
Member

Choose a reason for hiding this comment

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

Also is radius 0 allowed or not? The docs say less than zero, the code says zero or less. IMO zero radius should be allowed to get a scuffed vector (because people might make a shrinking circle and it would be nice to just go to zero)

Copy link
Member Author

Choose a reason for hiding this comment

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

If radius is less than 0 wouldn't this return error without setting an error?

Yes because that function is used inside functions that don't set errors ( specifically pgCircle_FromObject and in the future its fastcall version), so the error code 0 is forwarded to that function that then forwards its error code 0 and then sets the error. Like this:

static int
pg_circle_init(pgCircleObject *self, PyObject *args, PyObject *kwds)
{
    if (!pgCircle_FromObject(args, &self->circle)) {
        PyErr_SetString(
            PyExc_TypeError,
            "Arguments must be a Circle, a sequence of length 3 or 2, or an "
            "object with an attribute called 'circle'");
        return -1;
    }
    return 0;
}

Also is radius 0 allowed or not?

Right now radius=0 is not allowed since geometrically and mathematically that's a degenerate shape.

The docs say less than zero, the code says zero or less.

Yeah i didn't port the docs fully so the part that specified that it's not valid was missing. Now it's there.

IMO zero radius should be allowed to get a scuffed vector (because people might make a shrinking circle and it would be nice to just go to zero)

I'm not so sure about this tbh might cause issues with collision or other stuff.

Copy link
Member

Choose a reason for hiding this comment

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

We have zero vectors and zero surfaces. It seems more inconvenient to not be able to make it with zero radius.

This is not a blocker for this PR though.

@itzpr3d4t0r

This comment was marked as resolved.

Co-authored-by: Emc2356 <63981925+emc2356@users.noreply.github.com>
Co-authored-by: NovialRiptide <35881688+novialriptide@users.noreply.github.com>
Co-authored-by: ScriptLineStudios <scriptlinestudios@protonmail.com>
Co-authored-by: Avaxar <44055981+avaxar@users.noreply.github.com>
Co-authored-by: maqa41 <amehebbet41@gmail.com>
@Starbuck5
Copy link
Member

A question, should i add these to static.c?

I guess so.

Comment on lines +51 to +53
**Circle Attributes**

----
Copy link
Member

Choose a reason for hiding this comment

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

This strategy is unconventional versus the rest of the docs, gave me a pause when looking at the generated copy.

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

It's experimental, it's hidden. That makes it okay to merge.

Still have some open feedback but nothing critical.

This could be a really cool module!

test/geometry_test.py Outdated Show resolved Hide resolved
Co-authored-by: Dan Lawrence <danintheshed@gmail.com>
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Alright, this seems to work OK to me.

I think the main thing I would focus on next for circle is making it match some of the feature set of Rect/Frect. Particularly some of those familiar properties that make sense, e.g.:

x,y,r
center
diameter

where you can set one and it updates the other (e.g. I set diameter to X and radius becomes x/2).

Then I would make sure we have a good story for interoperability with pygame.draw.circle, compared to Rect/Frect:

pygame.draw.rect(screen, Color("red"), rect_1)
pygame.draw.circle(screen, Color("red"), circle_1)  # - Ideal, but not backwards compatible
pygame.draw.circle(screen, Color("red"), circle_1.center, circle_1.r)  # - Workable
pygame.draw.circle(screen, Color("red"), (circle_1.x, circle_1.y), circle_1.r)  # - Current situation

@MyreMylar MyreMylar added this to the 2.4.0 milestone Oct 8, 2023
@MyreMylar MyreMylar merged commit c4a17ec into pygame-community:main Oct 8, 2023
30 checks passed
@itzpr3d4t0r itzpr3d4t0r mentioned this pull request Jul 17, 2023
87 tasks
@itzpr3d4t0r itzpr3d4t0r deleted the add_circle_object branch November 4, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry pygame.geometry New API This pull request may need extra debate as it adds a new class or function to pygame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants