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

Replace get_surface method of Window with surface property #2632

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

Conversation

ankith26
Copy link
Member

I know get_surface was added with similarity to pygame.display in mind, but I think having this as a property instead can make things easier and neater for users of this API.

With this, the users would not need to keep track of surface of a window in a separate variable

@ankith26 ankith26 added New API This pull request may need extra debate as it adds a new class or function to pygame window pygame.Window labels Dec 25, 2023
@ankith26 ankith26 added this to the 2.5.0 milestone Dec 25, 2023
@ankith26 ankith26 requested a review from a team as a code owner December 25, 2023 08:54
@ankith26 ankith26 force-pushed the ankith26-window-surface branch from 9b68e96 to c1a66d7 Compare December 25, 2023 08:59
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.

This makes sense to me. Under the old system there was one display surface to rule them all, but now with multiple windows each special 'flippable' window surface is surely tied to a specific window and becomes invalid if that window is deleted. Making it a property should make this tight relationship between a specific window and a specific flippable surface clearer.

under the old system I used to always get my display surface straight from calling .set_mode() and I imagine many people did and won't particularly miss .get_surface() when switching over.

👍

Copy link
Member

@Matiiss Matiiss left a comment

Choose a reason for hiding this comment

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

Changes look good, the new API makes sense. Maybe as a property it could be moved above the classmethod in the docs, but that's getting deprecated soon enough so maybe doesn't matter.

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.

I'm skeptical of this, I've been slowly thinking about why off an on for a while.

We need to enforce separation of software rendering and hardware rendering, because if we don't SDL will throw lots of sometimes unclear errors.

With this change people don't have to "opt in" to using a display surface, from their perspective the display surface will always be there. Since it's a property it implies there isn't an action to get it, I feel that properties imply that the attribute is always present.

get_surface also has the advantage of having a very similar semantic to the existing pygame.display.get_surface

@ankith26
Copy link
Member Author

ankith26 commented Jan 23, 2024

I agree with the current system not enforcing a clear distinction between hardware and software rendering, but IMO get_surface does not help much in this regard either.
The best way to make a clear distinction is to actually make Window a base class (with not much changes to its existing structure), and have two distinct classes, maybe something like SoftwareWindow and HardwareWindow. SoftwareWindow will have things like .flip() and .surface property, whereas HardwareWindow could have things like a .renderer property?

@Starbuck5
Copy link
Member

That's an intriguing idea.

I'd suggest SurfaceWindow and RendererWindow as the names. We'd also need OpenGLWindow. (Potentially VulkanWindow and GPUWindow (SDL GPU API coming to SDL3) in the future)

@Starbuck5 Starbuck5 marked this pull request as draft April 29, 2024 05:53
@ankith26 ankith26 modified the milestones: 2.5.0, 3.0.0 May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame window pygame.Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants