Repository navigation
Add opt-in frame resolution scaling to the shared Vulkan renderer - #9
anxiousintrovert wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f766eb01b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| g.main_depth = CreateImage(render_width, render_height, 1, g.depth_format, | ||
| VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT | | ||
| VK_IMAGE_USAGE_TRANSFER_DST_BIT); | ||
| g.display_depth = CreateImage(width, height, 1, g.depth_format, | ||
| VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT | |
There was a problem hiding this comment.
When render_scale_factor != 1 and a display list uses a share_main_depth target, TargetPixelSize sizes its color image from g.main_color, while ResolveTarget pairs it with MainDepth() (g.display_depth) during the display pass. These allocations now give those attachments scaled and window extents respectively; above 1x the render area is larger than the depth attachment (invalid dynamic rendering), and below 1x depth/stencil coordinates address only a corner, breaking shared-depth effects such as shadows. The display pass needs a display-sized shared target or a matching depth image.
Useful? React with 👍 / 👎.
| for (std::thread &thread : threads) { | ||
| thread.detach(); |
There was a problem hiding this comment.
During renderer initialization, reaching done == total only proves each final pipeline call incremented done; the last worker can still be executing progress.notify_one() and the loop's next++. Detaching lets this scope destroy progress, next, and the other reference-captured locals concurrently, causing a startup use-after-lifetime. Keep these threads joinable and join them after the progress loop.
Useful? React with 👍 / 👎.
| TouchOverlayHandleEvent({touch_kind, std::numeric_limits<std::uint64_t>::max(), | ||
| touch_x / width, touch_y / height}, | ||
| true); | ||
| } | ||
| return; |
There was a problem hiding this comment.
On Android, when an SDL mouse event is from the touch/pen synthesized device (or no physical mouse is detected), this unconditional return runs even if the overlay is hidden, and the documented result of TouchOverlayHandleEvent is discarded when it is visible. An uncaptured touchscreen event therefore never reaches MouseHandleEvent below, so taps and motion outside the virtual controls cannot drive menus or other pointer UI; return only when the overlay actually captures the event.
Useful? React with 👍 / 👎.
| RecreateScaledMainTargets(); | ||
| RecreateRenderTargets(); |
There was a problem hiding this comment.
For a surfaced display-list renderer after a canonical render, SetRenderScale calls RecreateScaledMainTargets, which clears both main images, but leaves g.canonical_newest true. A subsequent PresentCanonical or noncanonical RenderList therefore accepts and presents the cleared image until another canonical tick; either preserve the canonical image while resizing or invalidate the flag so the caller rerenders it.
Useful? React with 👍 / 👎.
| mouse.dy = g_menu_dy + dy + g_scripted.mouse_dy + std::exchange(g_touch_dy, 0.0f); | ||
| mouse.wheel = MouseTakeWheel() + std::exchange(g_scripted_wheel, 0.0f) + std::exchange(g_touch_wheel, 0.0f); | ||
| mouse.buttons = MouseButtons() | g_scripted.mouse_buttons | g_touch_held; | ||
| mouse.buttons = MouseButtons() | g_scripted.mouse_buttons; |
There was a problem hiding this comment.
When menu mouse is enabled with a DualShock/DualSense touchpad, ReadTouchpad stores the pad click and Create-button states in g_touch_held via TouchpadStep::held, but this assignment no longer includes that state. Holding the touchpad or Create therefore never appears as a left/right click even though touchpad motion still works; include g_touch_held in the returned button mask.
Useful? React with 👍 / 👎.
| nullptr); | ||
| #ifdef __ANDROID__ | ||
| g.cmd_set_front_face(cmd, VK_FRONT_FACE_COUNTER_CLOCKWISE); | ||
| #else |
There was a problem hiding this comment.
There’s a few instances of weirdness like this, ifdefs with an else that’s the same as the first case. Can we get rid of these
Android builds import the NDK's Vulkan loader, ask for Vulkan 1.3 and compile the shaders for it. vulkan_android.cpp defines the twelve 1.3 commands the renderer calls, forwarding to the device's own, so no call site differs by platform. The swapchain leaves rotation to the compositor and keeps itself through the SUBOPTIMAL results that rotation reports while the surface is unchanged; a surface lost in the background is made again on a later frame. gfx errors also go to the system log. The window is fixed fullscreen in landscape. The game's on-screen gamepad passes its state to InputSetTouchOverlay, which pad 0 takes while InputTouchOverlayVisible() reports that no gamepad, keyboard or mouse is in use. Float parsing and the input script's step search avoid the parts of libc++ Android lacks, and the pipeline workers are joined std::threads. Desktop builds are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This is likely quite out of date and will need re-basing |
b08520d to
da1eac5
Compare
anxiousintrovert
changed the title
|
Closing for now as unnecessary: neither Chronicle nor ChronicleTwo calls an opt-in frame resolution scale (both use SetRenderScale), and it would need rework for the render thread. Happy to revisit with a consumer. |
Summary
Rebased onto current main (
04bd301) and narrowed to the remaining opt-in frame resolution scaling functionality. Upstream99f13eealready adopted the core Android support from this PR; this revision retains that implementation rather than replaying the old Android patches.RendererConfig::frame_scaleandgfx::SetFrameScale(float)for canonical and interpolated frame color/depth targets.render_scaleAPI keeps its semantics.This remains in the shared Vulkan renderer. No
ps2/files are modified. The API is available to all host platforms, but callers must explicitly opt in. Game UI scales with the frame; the Android native touch overlay remains at native resolution.Chronicle integration
The local Chronicle integration wires its Android resolution setting to this API under Android guards. That consumer change and the current upstream audio/API adapter were used for APK validation; they are not included in this Kazarov PR. A consumer update is required to enable this feature in Chronicle. The prior description's recommendation to close this PR as entirely superseded is replaced by this narrower scope.
Validation at da1eac5
Physical Fold behavior, Samsung screen recording, and sustained dungeon performance were not tested in this rebase pass. No performance improvement is claimed from these checks. Historical performance figures in the previous description applied to older revisions and are not validation of this head.