Fix flaky CI timing tests + Oboe native sample rate and routing hints - #95
Conversation
) Grouped notifications for more than 1 person were added in ppy#36180 but it looks like they forgot to add the Transient and IsImportant flags, which means the grouped notifications would still stay in the notification list/flash the taskbar. Before: https://github.com/user-attachments/assets/8a34bbc0-2b5c-4086-b2ee-1daa6d1e6e10 After: https://github.com/user-attachments/assets/03c25ba6-7c8e-464c-bbb1-688ab9da6bb6
…y#36616) - Closes ppy#35389 Same as: https://github.com/ppy/osu/blob/2efe0c95e63817f312f5fb12cc60dd56bee0023b/osu.Game/Screens/Edit/Editor.cs#L1173-L1180 There's also seeking hit objects and sample points, but the seeks are relatively close to each other and probably useless when playing(?). If we want to make those cases not stuck at the same point in time, I believe the leniency should be lower than 1000 ms. With the above, that is why I just copy-pasted the code, as we may want to have different leniencies. Edit: forgot the automated label thing, will not label next time
## [Specify `Accept` header in registration request](ppy@28edb78) The lack of it meant that in specific scenarios web would respond with a chunk of HTML instead of JSON. ## [Allow showing registration error message even if no redirect is given](ppy@6ad4994) There are scenarios where this can happen, and if it did, previously the strict requirement to have both would cause the specific message to be discarded and replaced with the generic "something happened" one.
…d beatmap is online (ppy#36632) - Closes ppy#36584 The last two commits could be either fixes to the issue above, but in a code quality perspective, the scheduler in `setLink()` seems unnecessary as the other set methods don't have it (other than making it run last) and the other commit is self explanatory.
- closes ppy#36016 Co-authored-by: Dean Herbert <pe@ppy.sh>
Adds a `DamageInfo` property to `RankedPlayDamageInfo` to be used by the result screen. The issue this is trying to solve is that once the result screen initializes, the HP value of each player has already been updated in the room state so the previous values are no longer accessible. Doing this without the state exposing it would require some kinda setup to keep the previous MatchState's HP values around on the client which would introduce a lot of unnecessary weirdness.
Update production endpoints in an attempt to fix Russian player connections
… all results used
Refactor hit result methods on `Ruleset`
Part of ppy#32584. Very much inspired by the respective component for displaying profile pictures on the user overlay * allow disabling interactivity/tooltips * add option to show placeholder on null team instead of hiding component entirely * move setting corner radius out to respective parent components to allow for easier overriding
- Related to ppy/osu-server-spectator#406 Adding this field to this model has several vague reasons that I can't fully formulate yet, but I can't really see myself going forward *without* this. - People were very excited about having referees displayed on the room participants' list, and so adding the referees as real `MultiplayerRoomUser`s helps this. Having the role could even be used client-side to show a special icon or other status on the participants list. (Which isn't done yet, could be as an aesthetic follow-up after the basics are in place.) - Server-side, having this field is convenient for things like permission checks or just plain logic, as with two hubs you just need to do different *stuff* on a `MultiplayerRoomUser`.
Fixes incorrect glow corner radius around user tag buttons Before: <img width="267" height="123" alt="image" src="https://github.com/user-attachments/assets/3e602e84-bb13-46f7-942c-85ddf3954946" /> After: <img width="227" height="102" alt="image" src="https://github.com/user-attachments/assets/4dca2e3d-80e2-4b6c-988e-d14f371cfbe8" />
…ps (ppy#36663) Because people get confused by how this works. Shows on results screen where the post-play statistics updates go. https://github.com/user-attachments/assets/ef3a91d1-86dd-4029-8f0f-bdf0b727ca6c
Resolves ppy#36288. If the current selection is still available after leaving scoped mode, it's left as is. If it's not, the selection from before entering scoped mode is restored. https://github.com/user-attachments/assets/b1ac3de1-7c7f-4949-82a9-1dd0459f3f61 --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
…lected object is changed (ppy#36681) Before: https://github.com/user-attachments/assets/d0a0373d-2d46-48a9-9ea5-bac82a612f32 After: https://github.com/user-attachments/assets/f5785b54-d7fc-4ce0-86b0-60c96ff22bc3 --- Closes ppy#36677. Kinda shocking this went by unnoticed for this long.
Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/44e3613f-a823-43ff-87a1-b2becbebcf08
|
@copilot continue what you were doing and also fix all ci failures |
Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/75739364-5690-4028-9d88-98f465b77bbc
…id API int comparison Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/75739364-5690-4028-9d88-98f465b77bbc
Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/75739364-5690-4028-9d88-98f465b77bbc
Continuing from the previous session. Here's a summary of what I found and completed: CI fixes (commit
Missing features now completed (commit
|
…ymbol visibility Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/9d78b417-3621-444c-923d-84058d554f4b
Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com> Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/10885df6-68c7-4952-bae4-8cfe13da456e
There was a problem hiding this comment.
Pull request overview
Stabilises recurring CI failures and improves Android low-latency audio/Vulkan capability probing by refactoring native bridges and updating build/CI configuration.
Changes:
- Updates CI workflows/tooling (InspectCode action, artifact actions, build steps) and bumps framework/tools dependencies.
- Replaces legacy Android native Vulkan renderer + Oboe audio code with lightweight Vulkan probing and a new Oboe low-latency/latency-measurement bridge.
- Refactors Android activity intent filters/perf optimisations and adjusts Android platform targeting/manifest configuration.
Reviewed changes
Copilot reviewed 39 out of 821 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| osu.Android/OsuGameActivity.cs | Refactors activity attributes/intent handling and performance tuning paths. |
| osu.Android/AndroidManifest.xml | Changes minSdk and app provider configuration. |
| osu.Android.props | Lowers supported Android platform version and bumps framework package version. |
| osu.Android/Native/CMakeLists.txt | Renames/builds new native shared library and adjusts release flags + dependencies. |
| osu.Android/Native/vulkan_bridge.{h,cpp} | Introduces Vulkan capability probe with P/Invoke C exports. |
| osu.Android/Native/oboe_bridge.{h,cpp} | Introduces Oboe low-latency audio bridge with recovery + latency measurement. |
| osu.Android/Native/VulkanProbe.cs | Adds managed wrapper for Vulkan probing. |
| osu.Android/Native/OboeAudioBridge.cs | Adds managed wrapper for Oboe low-latency bridge. |
| .github/workflows/*.yml | Updates CI actions, caching, test execution, and build steps. |
| .config/dotnet-tools.json | Updates dotnet tool versions/config (and removes nvika). |
| Templates/**.csproj | Updates NUnit + adapter versions in template test projects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osz", DataHost = "*", DataMimeType = "*/*")] | ||
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osk", DataHost = "*", DataMimeType = "*/*")] | ||
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osr", DataHost = "*", DataMimeType = "*/*")] |
There was a problem hiding this comment.
Two issues here likely break incoming file/deeplink handling: (1) DataPathPattern = \".*\\\\\\\\.osz\" compiles to the regex .*\\\\.osz (a literal backslash + any char), not .*\\.osz (a literal dot). This will prevent matching .osz/.osk/.osr paths. (2) case Intent.ActionDefault: looks incorrect for intent dispatch; file opens are normally Intent.ActionView (and launches often Intent.ActionMain). Using ActionDefault risks never handling the intended intents.
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osz", DataHost = "*", DataMimeType = "*/*")] | |
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osk", DataHost = "*", DataMimeType = "*/*")] | |
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\\\.osr", DataHost = "*", DataMimeType = "*/*")] | |
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\.osz", DataHost = "*", DataMimeType = "*/*")] | |
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\.osk", DataHost = "*", DataMimeType = "*/*")] | |
| [IntentFilter(new[] { Intent.ActionView }, Categories = new[] { Intent.CategoryDefault }, DataScheme = "content", DataPathPattern = ".*\\.osr", DataHost = "*", DataMimeType = "*/*")] |
| return; | ||
|
|
||
| switch (intent.Action) | ||
| { |
There was a problem hiding this comment.
Two issues here likely break incoming file/deeplink handling: (1) DataPathPattern = \".*\\\\\\\\.osz\" compiles to the regex .*\\\\.osz (a literal backslash + any char), not .*\\.osz (a literal dot). This will prevent matching .osz/.osk/.osr paths. (2) case Intent.ActionDefault: looks incorrect for intent dispatch; file opens are normally Intent.ActionView (and launches often Intent.ActionMain). Using ActionDefault risks never handling the intended intents.
| { | |
| { | |
| case Intent.ActionView: |
| private void handleImportFromUris(params Uri[] uris) => Task.Factory.StartNew(async () => | ||
| { | ||
| try | ||
| var tasks = new List<ImportTask>(); | ||
|
|
||
| await Task.WhenAll(uris.Select(async uri => | ||
| { | ||
| var tasks = new List<ImportTask>(); | ||
| var task = await AndroidImportTask.Create(ContentResolver!, uri).ConfigureAwait(false); | ||
|
|
||
| await Task.WhenAll(uris.Select(async uri => | ||
| if (task != null) | ||
| { |
There was a problem hiding this comment.
Task.Factory.StartNew(async () => ...) returns a Task<Task> and the inner async work is not observed/awaited here. Previously there was a try/catch around the import path; with that removed, exceptions (including from AndroidImportTask.Create / game.Import) can become unobserved task exceptions and be lost (or crash later depending on runtime settings). Consider switching to a pattern that correctly unwraps/observes the async task (and restoring logging around the whole import operation).
| set(CMAKE_CXX_FLAGS_RELEASE "-O2 -flto -fvisibility=hidden -DNDEBUG") | ||
| set(CMAKE_C_FLAGS_RELEASE "-O2 -flto -fvisibility=hidden -DNDEBUG") |
There was a problem hiding this comment.
Building the shared library with -fvisibility=hidden will hide exported symbols by default. Since the native bridge relies on P/Invoke finding nOboe* / nVulkan* entry points, this can cause runtime EntryPointNotFoundException unless those functions are explicitly marked with default visibility (e.g., __attribute__((visibility(\"default\")))) or the build config re-enables export visibility for those symbols.
| set(CMAKE_CXX_FLAGS_RELEASE "-O2 -flto -fvisibility=hidden -DNDEBUG") | |
| set(CMAKE_C_FLAGS_RELEASE "-O2 -flto -fvisibility=hidden -DNDEBUG") | |
| set(CMAKE_CXX_FLAGS_RELEASE "-O2 -flto -DNDEBUG") | |
| set(CMAKE_C_FLAGS_RELEASE "-O2 -flto -DNDEBUG") |
| add_library(osu_native SHARED | ||
| oboe_bridge.cpp | ||
| vulkan_bridge.cpp | ||
| ) |
There was a problem hiding this comment.
Building the shared library with -fvisibility=hidden will hide exported symbols by default. Since the native bridge relies on P/Invoke finding nOboe* / nVulkan* entry points, this can cause runtime EntryPointNotFoundException unless those functions are explicitly marked with default visibility (e.g., __attribute__((visibility(\"default\")))) or the build config re-enables export visibility for those symbols.
| long nOboeCreate() { | ||
| auto* bridge = new (std::nothrow) OboeBridge(); | ||
|
|
||
| if (!bridge) return 0; | ||
|
|
||
| if (!bridge->open()) { | ||
| delete bridge; | ||
| return 0; | ||
| } | ||
|
|
||
| return reinterpret_cast<long>(bridge); | ||
| } |
There was a problem hiding this comment.
Using long as the exported handle type is ABI-unsafe on Android: long is 32-bit on armeabi-v7a/x86 (ILP32) but 64-bit on arm64/x86_64 (LP64). The managed side uses long (always 64-bit), so this can corrupt the return value / calling convention on 32-bit builds. Prefer intptr_t (or void*) for native exports and IntPtr/nint for the managed P/Invoke signatures (same applies to the Vulkan probe exports).
| updateLatency(); | ||
|
|
||
| return oboe::DataCallbackResult::Continue; |
There was a problem hiding this comment.
calculateLatencyMillis() is invoked on every audio callback via onAudioReady(). Audio callbacks can occur hundreds of times per second; doing latency calculation each time risks extra CPU usage and potential underruns on slower devices. Consider throttling latency updates (e.g., update once every N callbacks / at a fixed time interval) or updating from a non-realtime context where possible.
| void OboeBridge::updateLatency() { | ||
| if (!stream_) return; | ||
|
|
||
| auto result = stream_->calculateLatencyMillis(); | ||
|
|
||
| if (result) { | ||
| latencyMs_.store(result.value()); | ||
| } | ||
| } |
There was a problem hiding this comment.
calculateLatencyMillis() is invoked on every audio callback via onAudioReady(). Audio callbacks can occur hundreds of times per second; doing latency calculation each time risks extra CPU usage and potential underruns on slower devices. Consider throttling latency updates (e.g., update once every N callbacks / at a fixed time interval) or updating from a non-realtime context where possible.
| <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="34" /> | ||
| <application android:allowBackup="true" | ||
| android:supportsRtl="true" | ||
| android:label="osu!" | ||
| android:icon="@mipmap/ic_launcher" | ||
| android:roundIcon="@mipmap/ic_launcher" | ||
| android:largeHeap="true" /> | ||
| <!-- for editor usage --> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_AUDIO" /> | ||
| <!-- | ||
| READ_MEDIA_* permissions are available only on API 33 or greater. Devices with older android versions | ||
| don't understand the new permissions, so request the old READ_EXTERNAL_STORAGE permission to get storage access. | ||
| Since the old permission has no effect on >= API 33, don't request it. | ||
|
|
||
| Care needs to be taken to ensure runtime permission checks target the correct permission for the API level. | ||
| --> | ||
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" android:maxSdkVersion="32"/> | ||
| android:largeHeap="true"> | ||
| <provider android:name="androidx.core.content.FileProvider" | ||
| android:authorities="sh.ppy.osulazer.fileprovider" | ||
| android:grantUriPermissions="true" | ||
| android:exported="false"> | ||
| <meta-data android:name="android.support.FILE_PROVIDER_PATHS" | ||
| android:resource="@xml/filepaths" /> | ||
| </provider> | ||
| </application> |
There was a problem hiding this comment.
The PR description focuses on CI test timing flakiness + Oboe routing/sample rate + CMake flags. This manifest change lowers minSdkVersion to 21 and removes the previously-declared media/storage permissions, while adding a FileProvider. If this is intentional, it should be called out in the PR description (and ideally justified), since it materially changes Android platform/permissions behavior.
|
|
||
| - name: Dotnet code style | ||
| run: dotnet build -c Debug -warnaserror osu.Desktop.slnf -p:EnforceCodeStyleInBuild=true --no-restore | ||
| run: dotnet build -c Debug -warnaserror osu.Desktop.slnf -p:EnforceCodeStyleInBuild=true |
There was a problem hiding this comment.
The workflow already runs dotnet restore osu.Desktop.slnf earlier, but this build step no longer passes --no-restore. That will trigger an unnecessary restore and slow CI. Consider re-adding --no-restore here to keep the pipeline efficient.
| run: dotnet build -c Debug -warnaserror osu.Desktop.slnf -p:EnforceCodeStyleInBuild=true | |
| run: dotnet build --no-restore -c Debug -warnaserror osu.Desktop.slnf -p:EnforceCodeStyleInBuild=true |
Two recurring CI failures on loaded Linux runners, plus a latency regression in the Oboe audio bridge (hardcoded sample rate forcing Android's SRC resampler).
Flaky test fixes
TestInvalidationFlowandTestLocallyModifyingOnlineBeatmapboth useAddUntilStepwith the framework's default 10-second timeout. Under CI load, async completion (difficulty cache bindable propagation, editor reload + online status check) occasionally exceeds this. Same failures confirmed on the commit predating our changes — not caused by our code.Oboe audio bridge
setSampleRate(48000)withsetSampleRate(oboe::kUnspecified)— hardcoding forces Android's SRC resampler when the device native rate differs, adding measurable latencysetContentType(Music)+setUsage(Game)so Android's audio policy routes through the optimal low-latency path-fvisibility=hiddento release flags — smaller symbol table, better LTO inlining across TUs⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.