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
920 stars 152 forks source link

Should we make "multi-draw' functions? #2208

Open Starbuck5 opened 1 year ago

Starbuck5 commented 1 year ago

Some of the oldest PRs in the PR list implement "multi-draw" functions, where it is a fast way to draw a bunch of polygons at once, or a bunch of circles at once.

These are the PRs I'm referring to, by @itzpr3d4t0r: #1841, #1834, #1835, #1839

I'd like to have a discussion about those PRs.

I'm skeptical we want to introduce a bunch of functions where the attraction is doing the same thing as another function but with less function call overhead.

I don't want to use a slippery slope argument, but the same logic around these draw functions applies to many other places in the codebase. If we want a multi polygon draw, why wouldn't we want a multi surface transform scale, or a multi image rotate, or a multi text render (I feel like these are not good examples but it's late for me). I guess we already have blits and colliderects, and those seem reasonable.

In the case of draw.polygons, the provided test case uses a very small and low impact polygon. (triangle, width=2, dim=20) and shows a good improvement there. But of course larger more complex polygons, filled polygons make the performance impact negligible eventually. I'm not sure what my point with that is, I'm sure everyone is aware of that.

Is it worth trying to spruce up the draw module in this way while we're not sure how drawing and _sdl2.video will work together?

Is there a compelling use case for polygons (3d rendering enthusiasts?) that means it should exist and nclines / circles shouldn't?

MyreMylar commented 1 year ago

The classic use case I have in mind when considering draw.polygons is the software 3D wireframe renderer that many people seem to create as a learning exercise (I've seen lots of these, usually with a spinning teapot, posted on Reddit).

I would be curious to know if switching from draw.polygon to draw.polygons (in a sensible manner) makes a performance difference for that case. I know that users often struggle with frame rate in their 'software' 3D renderers so if they are being held up by the drawing specifically then this could help. It may be that other calculations are actually the dominant factor and drawing the list of polygons for a frame is irrelevant.

On Tue, 30 May 2023, 09:15 Charlie Hayden, @.***> wrote:

Some of the oldest PRs in the PR list implement "multi-draw" functions, where it is a fast way to draw a bunch of polygons at once, or a bunch of circles at once.

These are the PRs I'm referring to, by @itzpr3d4t0r https://github.com/itzpr3d4t0r: #1841 https://github.com/pygame-community/pygame-ce/pull/1841, #1834 https://github.com/pygame-community/pygame-ce/pull/1834, #1835 https://github.com/pygame-community/pygame-ce/pull/1835, #1839 https://github.com/pygame-community/pygame-ce/pull/1839

I'd like to have a discussion about those PRs.

I'm skeptical we want to introduce a bunch of functions where the attraction is doing the same thing as another function but with less function call overhead.

I don't want to use a slippery slope argument, but the same logic around these draw functions applies to many other places in the codebase. If we want a multi polygon draw, why wouldn't we want a multi surface transform scale, or a multi image rotate, or a multi text render (I feel like these are not good examples but it's late for me). I guess we already have blits and colliderects, and those seem reasonable.

In the case of draw.polygons, the provided test case uses a very small and low impact polygon. (triangle, width=2, dim=20) and shows a good improvement there. But of course larger more complex polygons, filled polygons make the performance impact negligible eventually. I'm not sure what my point with that is, I'm sure everyone is aware of that.

Is it worth trying to spruce up the draw module in this way while we're not sure how drawing and _sdl2.video will work together?

Is there a compelling use case for polygons (3d rendering enthusiasts?) that means it should exist and nclines / circles shouldn't?

— Reply to this email directly, view it on GitHub https://github.com/pygame-community/pygame-ce/issues/2208, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGDGGU2E2UEXUO3T7Q3MLLXIWUDNANCNFSM6AAAAAAYTULR2Y . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Starbuck5 commented 1 year ago

I just asked someone making a 3d renderer to test Scriptline's PR https://github.com/pygame-community/pygame-ce/pull/2126, which is a 50% increase in triangle draw speed.

They reported that polygon drawing went from 5.1% of runtime down to 4.4%.

So a little improvement, but not significant on its own to their end result.

itzpr3d4t0r commented 1 year ago

The primary reason for considering the addition of these functions is simply filling the gaps since we already have draw functions for multiple shapes (draw.lines and draw.aalines) and because of the general trend that pygame started. Specifically, I'm talking about all Rect functions that accept sequences, blits, the newly added fblits, and more.

I'm skeptical we want to introduce a bunch of functions where the attraction is doing the same thing as another function but with less function call overhead.

I believe this assumption is somewhat superficial. Functions that rely heavily on function call overhead due to their simplicity in logic greatly benefit from optimizations such as the speed of C or fast sequence looping (consider Rect.collidelistall() as an example). Again, you could make this very same assumption for blits or fblits since surfaces can be of various size and most of the function's performance is making up for the otherwise present overhead.

In the case of draw.polygons, the provided test case uses a very small and low impact polygon. (triangle, width=2, dim=20) and shows a good improvement there. But of course larger more complex polygons, filled polygons make the performance impact negligible eventually. I'm not sure what my point with that is, I'm sure everyone is aware of that.

I have always made this clear, so I suppose it's common knowledge. This is true for blits as well.

I don't want to use a slippery slope argument, but the same logic around these draw functions applies to many other places in the codebase. If we want a multi polygon draw, why wouldn't we want a multi surface transform scale, or a multi image rotate, or a multi text render (I feel like these are not good examples but it's late for me). I guess we already have blits and colliderects, and those seem reasonable.

Functions like colliderects are primarily limited by overhead, making them sensible to include, especially since games often test for collisions with multiple objects rather than a single one. While it is possible to use a single colliderect, we must acknowledge that games are often more complex. I suppose these functions are more of an "advanced" feature, as most beginners would simply use the singular blit or colliderect functions in a loop for simplicity, without necessarily optimizing their game code from the start. Therefore, keeping this in mind, it always makes sense, in my opinion, for a library to provide its userbase with simple and straightforward functionality and then evolve to assist more advanced users who require additional capabilities from the same library.

JiffyRob commented 1 year ago

What if instead of crowding the API with a dozen new functions, we used C's function overloading to take a sequence of args instead? eg Surface.blit((source, dest, area, flags), (source, dest, area, flags)) or pygame.draw.polygon((surface, color, points, width), (surface, color, points, width) Pros:

Cons:

itzpr3d4t0r commented 1 year ago

What if instead of crowding the API with a dozen new functions, we used C's function overloading to take a sequence of args instead? eg Surface.blit((source, dest, area, flags), (source, dest, area, flags)) or pygame.draw.polygon((surface, color, points, width), (surface, color, points, width) Pros:

  • no loads of new functions
  • might scale better as more and more functions do this

Cons:

  • would be inconsistent with current blits and such
  • not that much less work codewise (I think. I'm not so good with C, so I wouldn't know)

Actually this is a VERY good idea, but your examples actually made me think of a solution that's a bit better and easier (especially on the typehints' side).

Basically functions could also take as a first argument a sequence of tuples representing the corresponding arguments to the function so that the function would simply scan if the first arg is a sequence and if so run a loop on that sequence.

MyreMylar commented 1 year ago

Actually this is a VERY good idea, but your examples actually made me think of a solution that's a bit better and easier (especially on the typehints' side).

Basically functions could also take as a first argument a sequence of tuples representing the corresponding arguments to the function so that the function would simply scan if the first arg is a sequence and if so run a loop on that sequence

Should we move towards closing all the draw function PRs with an added 's' on them for this new idea? I don't want to review them all if they are about to be deprecated in favour of an improved plan.

My experience in general with these 'less overhead, per operation' PRs is that it makes a difference were you are likely to draw/blit lots of small copies of the thing but not so much difference when you spend more time in the drawing/blitting part of the code than in the setup. But if we could have the performance gains for multi-draw without adding new functions I would be more firmly in favour.

I think part of what I don't like about the sequence PRs is the overall vibe of the documentation and code autofill with lots of repeated text and params just for that little 's' of difference. If we could get this working we could even go back and undocument the existing draw.lines and draw.aalines (so they still work but we point users toward the new multi-use singular functions).

itzpr3d4t0r commented 1 year ago

Alright, I'll close my PRs. I guess I'll try and reimplement this functionality as we discussed, so in single use functions.