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
770 stars 120 forks source link

Add color argument to Surface constructor #2834

Closed damusss closed 2 months ago

damusss commented 2 months ago

I added the C logic, the docs, stubs and tests for the color Surface argument. I tested locally most cases and they work for me. I'm not too good with C so I hope I didn't mess up anything. I didn't lock the surface when filling it as a few lines above it wasn't locked either.

As always the same question, why?

Tell me what you think of this and maybe why you don't think it's a good addition.

Starbuck5 commented 2 months ago

It causes no harm

Actually, I believe that adding another keyword argument does do a small amount of harm. Because it's not free to have a keyword, even if it's not called.

import time
import pygame
pygame.init()

start = time.time()
for _ in range(1000000):
    a = pygame.Surface((14,5))
print(time.time() - start)

On main this usually runs in about 0.36 seconds for me, but on this PR it's more like 0.38. A small difference, but I'm confident it is there, it sticks around through various rounds of testing.

damusss commented 2 months ago

@Starbuck5 I guess that you are right. But what if, there was no argument but there was an extra overloading Surface(size, flags, color) it would be the same as the surface overload, if we don't find an int (depth) Surface (surface) we can assume they tried passing a color. I guess that ints are also valid color values but they seem to only effect the blue channel and they are weird so we can probably exclude that case for this argument. Not sure tho, tell me what you think :)

Starbuck5 commented 2 months ago

A slight performance loss is not what I'm worried about, I'm worried about whether it's a good API or not. I'm just being pedantic on your claim of no harm

I think I'm against this change right now: it makes the Surface code and API more complex for minimal benefit. If a user really wants to construct a surface and fill it with a color inline, they can do so with a trivial helper function.

I've seen a few people bring up the possibility of doing this before, but it often seems to come from a code golf mindset, which is not what we want to design an API around.

narilee2006 commented 2 months ago

You can run python setup.py format to automatically format your changes.

robertpfeiffer commented 2 months ago

I do not see the upside here.

itzpr3d4t0r commented 2 months ago

This got discussed and tentatively implemented already in https://github.com/pygame-community/pygame-ce/pull/2416. So I'd suggest closing this PR.