Repository navigation
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
Contributor
|
Windows check of head Host: Windows 11 (10.0.26300), Ryzen 5 9600X, WinLibs GCC 15.2.0, Release. GPUs: NVIDIA RTX 5070 Ti (617.42) and AMD Radeon RDNA2 iGPU
AI-assisted: yes (Claude Code); results above were run on this machine. |
MarcZX10
suggested changes
Oct 22, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
The report includes useful same-GPU validation and a clear one-pixel scope, but I found a path-classification issue that needs correction before this is ready. The guard below treats every non-mesh point-list pipeline as a vertex pipeline; the current shader-stage model also has geometry and tessellation paths. Please restrict this behavior to the vertex path (or implement and test the relevant downstream point-size behavior), with a regression that exercises a non-vertex point-list path.
The branch is based on 0c6a5bd5938f and is 423 commits behind current main (da54c652e30b); the checklist's current-main claim is stale. Please rebase, rerun conventions/overlap checks, and rerun the paired focused AGC validation against current main and this head. The existing Windows/NVIDIA test report is valuable, but it is on the older base.
The overlap report also identifies direct alternatives #6639 and #6801 in the same point-list state logic. This PR acknowledges that they are alternatives, but the contribution rules require author coordination on which goes first and recording the resulting order in Depends on. Please resolve that with the affected authors and refresh the overlap report after rebasing. I did not find a matching local AGC build or Vulkan device here, so I did not independently run the GPU tests. The hosted product jobs were skipped; policy checks passed.
AI assistance was used to prepare this review.
| zero(cx, 0x207, ~(LayerExports | ClipCullExports | inactiveVertexPointSize(queue)), "layer, viewport or auxiliary vertex exports"); | ||
| Require(std::popcount(read(cx, 0x207) & 0xffffu) <= 8, "more than eight clip and cull distances are unsupported"); | ||
| result.paClVsOutCntl = read(cx, 0x207); | ||
| if (result.topology == VK_PRIMITIVE_TOPOLOGY_POINT_LIST && !result.stages.mesh.has_value()) { |
Contributor
There was a problem hiding this comment.
This condition does not establish the vertex path: !result.stages.mesh is also true for the Geometry, Tessellation, and TessellationGeometry paths in ShaderStages. As written, the fixed-one-pixel register checks newly accept those point-list draws too, even though this change only establishes the no-PointSize vertex case and maintenance5's default does not replace a point size explicitly written by a downstream shader. Could this be gated on result.stages.path == ShaderPath::Vertex (and covered with a geometry/tessellation point-list regression), or otherwise handle those stages' point-size output explicitly?
12 tasks done
12 tasks done
Author
|
Superseded by #6801, now merged into main. It provides broader vertex and geometry point-list support, including register and per-vertex point sizes, so this minimal one-pixel alternative is no longer needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Mafia: Definitive Edition (PPSA36458) aborts while entering its main menu with
AGC graphics: point-list vertex rendering requires point-size output support. Allow vertex-path point lists whenPA_SU_POINT_SIZErequests one-pixel points andPA_SU_POINT_MINMAXincludes that size. RequireVK_KHR_maintenance5, which defines a default point size of 1.0 when the shader does not write PointSize.This is an intentionally minimal alternative to #6639, which implements register and per-vertex point sizes. It also overlaps #6801 at the point-list checks. Please choose one approach; these are overlapping alternatives, not dependencies. This version adds no shader request fields, shader ABI changes or pipeline-journal format changes. Larger points, per-vertex sizes and point-sprite texture-coordinate replacement remain explicit failures, documented in technical debt. The draw register key includes the point size, limits and sprite-control registers so cached decoding cannot bypass those checks.
Add state-decoding coverage for supported sizes, unsupported sizes and limits, missing registers, sprite replacement and the device-feature guard. Extend the GPU polygon-mode test to draw a vertex point list and verify the exact three output pixels.
Tested
Windows 11, WinLibs MinGW-w64 GCC 15.2.0, Release. NVIDIA GeForce RTX 4070 Laptop GPU, NVIDIA driver 617.42.
Compared current main
0c6a5bd5938fde7bbc0bf720927c4584bd3e4700with this change6fb20a01fon the same machine and GPU. Both runs passed all three focused tests:agc_driver_graphics,agc_driver_polygon_mode_draw, andagc_driver_inactive_vertex_point_size.Both verbose logs confirm insertion of
VK_LAYER_KHRONOS_validation; neither containswas not found but was requested by env var VK_INSTANCE_LAYERS. Main: zeroVUID-errors. Change: zeroVUID-errors, so no new validation errors. The layer was loaded from a local LunarG SDK extraction viaVK_LAYER_PATH.Local title run: after dismissing the 2K connection-failure dialog, Mafia reaches the rendered main menu and stays running until the diagnostic run's 180-second time cap. The contributor subsequently confirmed starting a new game. Performance remains poor: the contributor reports approximately 9-14 FPS in the opening cinematic and 1.2 FPS in gameplay. These are unprofiled observations, not a benchmark or a speedup claim; this PR addresses the startup abort.
No matching Game bring-up discussion for PPSA36458 was found, so the formal Unblocks item below is
none.python tools/check_conventions.py --base origin/mainpasses. The overlap report confirms #6639 and #6801 and reports a draw-key overlap with #6977 and constructor-anchor conflicts with #8564, #9020 and #9335; some other PRs cannot be compared because they already conflict with main. These line overlaps are not dependencies.Checklist
main; no other open PR implements the same functions (explicit exception: this is a deliberately overlapping minimal alternative to feat(agc): draw point lists on the vertex path with their point size #6639 and feat(agc): rasterize geometry points with their point size #6801, disclosed above)NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)