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

Refactor DisplayHandle into seperate android and linux properties #6268

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Apr 29, 2024

The DisplayHandle is specific to linux window magnagers and is very different from the android surface handle.
I'm not sure if any of this code actually works / is useful, as android doesn't use veldrid yet.

The `DisplayHandle` is very different from the android surface handle.
@bdach
Copy link
Collaborator

bdach commented Apr 30, 2024

The DisplayHandle is specific to linux window magnagers and is very different from the android surface handle.

In that case it should be annotated with [SupportedOSPlatform("linux")]. Probably in this diff.

Comment on lines +142 to +143
[SupportedOSPlatform("android")]
public virtual IntPtr SurfaceHandle => throw new PlatformNotSupportedException();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I hate that this exists but I see why. It's because the layering on this is kinda borked.

tl;dr: the graphics surface implementations live in base framework rather than the android project, so they can't directly reference something like AndroidGameActivity to bypass this song-and-dance entirely and just call what they want to call directly where they need it.

I guess with the SupportedOSPlatform attribute on this I could be okay with this but it feels a bit smelly nevertheless. There's probably a refactor in here that gets rid of it (likely by shipping the surface implementations off to the mobile projects - or at least the android one) but I'm not sure if worth.

@smoogipoo interested in your thoughts on layering here since this touches the veldrid parts

Copy link
Contributor

Choose a reason for hiding this comment

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

Not worth overthinking this.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 30, 2024
@Susko3 Susko3 changed the title Add IAndroidGraphicsSurface.SurfaceHandle instead of overloading DisplayHandle Refactor DisplayHandle into seperate android and linux properties Apr 30, 2024
@smoogipoo smoogipoo merged commit d73af6c into ppy:master Apr 30, 2024
17 of 19 checks passed
@Susko3 Susko3 deleted the dont-overload-DisplayHandle branch April 30, 2024 19:52
peppy added a commit to peppy/osu-framework that referenced this pull request May 21, 2024
…Handle"

This reverts commit d73af6c, reversing
changes made to f7718ea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants