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

Add SurfaceWindow subclass, remove borrowed window logic #2830

Open ankith26 opened 5 months ago

ankith26 commented 5 months ago

This is a draft PR to get feedback on the API and idea, so I have not updated docs/tests/types

Related to issue #2603

How to test this PR?

Use the example written by @Starbuck5 in https://github.com/pygame-community/pygame-ce/pull/2626, but have to just replace the two lines that construct Window and instead use SurfaceWindow

robertpfeiffer commented 5 months ago

hmm. There is no abstract Window class in this code. Doesn't is defeat the whole purpose?

If you remove the borrowing logic, you have to remove from_window from Renderer as well.

ankith26 commented 5 months ago

If you remove the borrowing logic, you have to remove from_window from Renderer as well.

Yeah I though of doing it but didn't because I didn't understand its purpose well enough. Thanks for letting me know I will remove it

Starbuck5 commented 5 months ago

So @ankith26 you first brought up the "window subclass" idea a few months ago and I've been tossing it around in my head since. You brought it up back in https://github.com/pygame-community/pygame-ce/pull/2632 when thinking about a .surface property.

I've been doing some thinking on it, and they are very interesting ideas, but I don't think we should implement either of them at this time.

Reasoning:

Also if we ever implement this I think it would be good to do the subclasses in Python, rather than C. Makes it easier to write and maintain. (I think blubber brought this up at some point too, to give him some credit for this idea as well)

damusss commented 5 months ago

I have a few things to say about Starbuck's comment, about the reasoning 1) as far as I remember Window is an experimental API, people are aware it might get modified in the future, it's not on us. plus they would really just change the class name. plus plus the more we wait the more are gonna have to change in the future. I think it's ok to forward this to pygame 3. 2) I don't think it's that bad to do things slightly different than SDL, pygame users aren't aware of the underlying sdl code and aren't supposed to (plus pygame is not just a wrapper)

About python subclassing, that's up to whoever implements it but for now it doesn't seem like it's too messy implemented in C. That's just my opinion, have a nice day :)

Starbuck5 commented 5 months ago

as far as I remember Window is an experimental API, people are aware it might get modified in the future, it's not on us. plus they would really just change the class name. plus plus the more we wait the more are gonna have to change in the future. I think it's ok to forward this to pygame 3.

People are aware in theory, but I've already seen complaining about tiny things changing in between releases with _sdl2. But the users are not my only concern, we want to push the Window API sooner rather than later, and a big change to the API like this shouldn't be happening at the stage I would like us to be in its development.

I don't think it's that bad to do things slightly different than SDL, pygame users aren't aware of the underlying sdl code and aren't supposed to (plus pygame is not just a wrapper)

Yes, I agree that pygame-ce is not just a wrapper, as people sometimes say. However, if we're not confident about doing something differently, it's not a good idea to do so from a maintenance perspective. Sticking with SDL concepts and SDL API design is a good insurance policy to make sure our stuff keeps working the way we expect.

damusss commented 5 months ago

@Starbuck5 To conclude this, I basically agree and i think we should keep developing this and eventually release it with pygame 3.0 among other OOP related ideas like python event types.

robertpfeiffer commented 5 months ago

I basically agree with Starbuck. I don't think doing it like this would be that bad, but it probably will be worse than what we have.

ankith26 commented 4 months ago

I have put this PR up for the 3.0.0 milestone. We don't have a consensus to do a change like this at this point so I'm fine with this PR being stalled for the foreseeable future, though I will leave this PR open so we can decide in the long term. For now, let's not halt Window progress. This plan can be re-introduced in a backwards compatible fashion if and when needed