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

Add SurfaceWindow subclass, remove borrowed window logic #2830

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ankith26
Copy link
Member

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 #2626, but have to just replace the two lines that construct Window and instead use SurfaceWindow

@ankith26 ankith26 requested a review from a team as a code owner April 28, 2024 10:13
@ankith26 ankith26 marked this pull request as draft April 28, 2024 10:17
@robertpfeiffer
Copy link
Contributor

robertpfeiffer commented Apr 28, 2024

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
Copy link
Member Author

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

cdef Renderer self = cls.__new__(cls)
self._win = window
if self._win._is_borrowed:
self._is_borrowed=1
Copy link
Contributor

Choose a reason for hiding this comment

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

This old code was incorrect anyway, because theoretically you might have a Window that isn't borrowed and end up with a borrowed Renderer. The question is why don't you use your existing reference to that renderer, but oh well.

If you can't borrow a Window though, borrowing a Renderer makes even less sense.

.tp_doc = DOC_WINDOW,
.tp_methods = surfacewindow_methods,
.tp_init = (initproc)surfacewindow_init,
.tp_new = PyType_GenericNew,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like my comment got lost. This .tp_new is correct, but the superclass must be abstract now

@Starbuck5
Copy link
Member

Starbuck5 commented Apr 29, 2024

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 #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:

  • People have already started using the API, big changes now are hard. We should focus on gaining confidence in what we have and releasing it out.
  • When in doubt, follow SDL. SDL2 has one Window type, SDL doesn't have a type level way to stop people from mixing software and hardware rendering (and they could, even in C).
  • SDL3 adds a new windowing concept with SDL_CreatePopupWindow. Us saying that the rendering type is part of the window object closes the door having "pop up window vs normal" be the type, or something else entirely.
  • We can add this later as a backwards compatible change, maybe when SDL3 is out, and we can be more confident about doing something new with the API

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
Copy link
Contributor

damusss commented Apr 29, 2024

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
Copy link
Member

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
Copy link
Contributor

damusss commented Apr 29, 2024

@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
Copy link
Contributor

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.

@yunline yunline added the window label May 8, 2024
@ankith26 ankith26 added this to the 3.0.0 milestone May 18, 2024
@ankith26
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants