Repository navigation
Conversation
Joey-Magnus
left a comment
There was a problem hiding this comment.
The change itself looks right to me, and the description is unusually thorough. What I checked in the source:
- Device setup. Only
shaderBufferFloat32AtomicAddis enabled after the query, and the features struct is reset with itspNextkept (gpu/vk/vk_main.cpp:383-394).RX_VK_NATIVE_FADD=0keeps the extension off entirely. - When the native path is chosen.
native_f32_addis decided inDefineStorageBuffers, before its early return (spirv_emit_context.cpp:1827-1834). The constructor callsDefineStorageBuffers(line 585) beforeDefineGlobalMemoryFunctions(line 592), so the global fallback sees the same answer. - No dangling function.
f32_add_casis only skipped when native, and its only user isEmitStorageAtomicAddF32, which takes the native branch first. - The global fallback walks the F32 views with their bound length, as the U32 ones are walked. The PSB path takes the native add only at
gp_level >= 3, as before. - The program-cache stamp includes the answer, so a native-add cache is never read on a GPU without the feature.
I ran test_native_float_atomic_add, test_global_atomic_add_f32, test_cas_loop_ceiling, test_amd_subgroup_profiles and the program_cache checks at the PR's head with --strict (spirv-val present), and all pass. On GitHub only the title check has run so far; the SPIR-V fixtures job will run the new test once a maintainer approves the workflows.
should fix (testing): by your own reading of the drivers, the native path turns on by default for AMD RDNA 3 and later, on RADV and on AMD's Windows driver. Those are RX 7000 and RX 9000, which docs/workflow.md puts in Tier 1 ("test here before merge"). It also turns on for Intel Arc (Tier 2). The only in-game runs are on an RTX 4090. The gain is about 0.5 ms of GPU time at a 60 fps cap, a polish-level win. A driver bug in float atomics on AMD, by contrast, would be a Tier 1 regression in the very compute passes (sky light, Kakariko) that are hard to tell apart visually. Please get one run on an RDNA 3/4 card (Linux RADV, ideally Windows too) in Kakariko and on Death Mountain Trail, with the log line and a picture, before this merges. #2202 shows there are RX 7900 XT testers around. If that can't happen soon, a smaller step is to enable it by default only on NVIDIA's driver for now (dev.props.vendorID/driverID) and widen it once AMD has a row in tested configurations.
minor: the branch conflicts with main in docs/tested-configurations.md, where both changes add rows at the top of the table. A rebase is enough. Keep your rows under any newer ones, since the table is newest first.
Joey-Magnus
left a comment
There was a problem hiding this comment.
The code is right: the feature is enabled only when queried and is in the stamp, and test_amd_subgroup_profiles, test_global_atomic_add_f32 and test_cas_loop_ceiling pass. Remaining:
- The conflict is only
docs/tested-configurations.md. - The native path is on by default for RDNA 3/4 and Arc, but only tested on an RTX 4090. Either get an RDNA 3/4 run or limit it to NVIDIA for now.
RX_VK_NATIVE_FADDusesatoi, so=onturns it off. Useenv_value_off.tests/test_amd_subgroup_profiles.pyconflicts with #2343, where both add adevfield. Keep both.
On Intel GPUs under Mesa, the game's culling programs never finish compiling, and Intel builds compute pipelines one at a time (issue 496), so every compute build queued behind them waits for good. In Kokiri Forest on an Iris Xe that leaves the forest grey, with no foliage or light, and the rocks and doors the mesh fallback draws stay white. Part of #999 and #1001 ## Why it happens A global memory access the storage buffer pass could not bind is translated into a search of the storage buffer tables: four tables of 16 entries, about 70 entries in all, each written out in `DefineGlobalMemoryFunctions`. These helpers are marked `DontInline` for NVIDIA, but Mesa inlines every function anyway. The foliage culling program 53caac5f has 46 global accesses, so it ends up with over 100k instructions. At the 32 lanes compute is pinned to, Intel's compiler (`brw`, register allocation at SIMD32) did not finish it in 5+ minutes. ## What changed - `spirv_emit_context.cpp`: when `global_table_loop` is set, each table is searched in a loop the driver is asked not to unroll (`DontUnroll`). The loop runs the same test as the written-out form, entry by entry: the same constant-buffer words, the same masked address and bounds, and the first match taken. Its counter is the same in every invocation that runs it, so this is dynamic indexing, not non-uniform, and modules with the loop declare `StorageBufferArrayDynamicIndexing`. The CAS loops' callers open their own block and keep the written-out form. - `vk_coopmat.cpp`: `table_loop_wanted()` turns it on for Intel's Mesa driver (`VK_DRIVER_ID_INTEL_OPEN_SOURCE_MESA`), only where the device has `shaderStorageBufferArrayDynamicIndexing`. The cause is Mesa's, so Intel's Windows driver, whose compiler is not Mesa's, keeps the written-out form until it has been tried. `RX_VK_GM_LOOP` overrides the default on any GPU, read as every switch is (`env_value_off`). - `vk_shader.cpp`: `translator_stamp()` takes the switch (flag bit 8). Programs cached in one form are not used in the other. The bit is 0 everywhere the loop is off, so on other GPUs the SPIR-V, the stamp and the cache are unchanged. - `docs/development.md`: a row for `RX_VK_GM_LOOP`. The effect on the four table-heavy programs: 53caac5f's SPIR-V halves (998 KB to 448 KB), and each now builds in 12-15 s instead of not finishing. ## Tests - `tests/test_global_table_loop.py` (new): - compiles `table_loop_wanted` and runs it for Intel's Mesa and Windows drivers, NVIDIA and AMD, with and without dynamic indexing and `RX_VK_GM_LOOP` (`1`, `true`, `0`); - checks that `translator_stamp` takes the switch; - checks that the loop reads the words the written-out form reads (cbuf_offset / 8 + 2k for every offset and entry) and runs the same bounds test. - `tests/global_load_16_ir.cpp`: the global load fixture emits one module whose storage buffer table is searched in the loop, so the SPIR-V fixtures job runs `spirv-val` on the loop's form (its counter, the return inside its body). - The checks that read the same sources pass: `python -I -B tests/run_all.py global_table_loop bake_mimo amd_fragment_warp bake_skeleton bake_ds cas_loop_ceiling global_atomic_add_f32 mesh_point_topology prepared_stamps recorder_sync program_cache_format program_stamp_knobs` gave 12 passed on Linux. `test_mesh_unreachable_outputs` was skipped (no `spirv-val` here). ## Tested in the game Iris Xe (Raptor Lake-P, i9-13900H, 32 GB), Mesa ANV 26.2.4, Bazzite. Kokiri Forest, walking out of Link's house: - Before: 53caac5f still compiling after 5+ minutes, and every compute build queued behind it waiting. The forest stays grey and the mesh-drawn rocks, doors and trees stay white. - After: every compute program the forest dispatches builds and runs, and the forest gets its foliage and light. With everything built: about 17 fps in the forest, 52 ms of GPU time per frame (`RX_VK_PASS_TIMING=2`), 20 ms of it the game's own dispatches. That work was never run before, so earlier frame rates in this scene are not comparable. Once the culling builds, the mesh fallback's colour draws still wait for their own (slow) builds. Until then the grass shows as sky-coloured holes, which bloom blows up into specks. #2382 holds those draws' depth while their colour programs build. **Not tested:** - Other Intel GPUs (Arc, UHD). - Windows: untested for now. Intel's Windows driver keeps the written-out form; `RX_VK_GM_LOOP=1` turns the loop on there. - GPUs where `RX_VK_GM_LOOP=1` is set by hand. - Intel users' program caches are rebuilt once, because the stamp changes. **Overlap:** #2306 also edits `DefineGlobalMemoryFunctions` (the float atomics). The code merges cleanly with it; only the `docs/development.md` row conflicts. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
This now conflicts with main in |
ecf6c6f to
50579ba
Compare
|
Thanks for both reviews, and for running the fixtures with
Rerun on the new head: the branch's tests pass with g++ and clang++ under The whole suite and the two game runs were made on the first rebase (7219b3fd on a5b955f); on the head that is pushed, 50579ba, I reran the branch's tests and the program cache's checks, and the three commits' code is unchanged from that rebase. |
|
Tested on Windows, which the description lists as not tested: RTX 5080 16 GB, NVIDIA 621.14, Windows 11 (build 26300), Ryzen 9 9900X3D. A build of this branch (50579ba) rebased locally onto main 87991bc (no conflicts), headless, the game's default output 2048x1152, Resolution 100%, the 60 fps cap with normal pacing; a save loaded, then teleported and Link left standing, 150 s in each place,
|
|
Let's get someone to test on AMD please |
|
@hckrman101 yes. This PC cannot do it (an RTX 4090, and an Intel UHD 770 whose Mesa has no buffer float add), so here is what an AMD run needs, for whoever has the card. What this pull request does on AMD today: nothing by default. The native add is chosen only with NVIDIA's driver; on any other GPU the loop stays and the log says Which cards: RDNA 3 or RDNA 4 (RX 7000 / 9000). RDNA 1 and 2 do not report The run (a build of this branch, or of main with it merged):
What decides it: the same picture and equal or less GPU time with the switch, and no |
|
Adult numbers, as promised: RTX 5080, Windows 11. Same build as before but rebased locally onto main c544828 (no conflicts), headless, the 60 fps cap with normal pacing. Loaded the save on Death Mountain Trail, switched to adult Link with the bridge's new
|
…t a compare-and-swap loop The game's RED/ATOM.ADD.F32 were always a compare-exchange loop, which gives up after CAS_LOOP_MAX_TRIES (1024) attempts and then loses the add. Where the device has VK_EXT_shader_atomic_float's shaderBufferFloat32AtomicAdd, the add is one OpAtomicFAddEXT on an f32 view of the buffer, in the storage buffers' emitter and in the global-address fallback; the loop stays elsewhere. Only that feature is enabled; RX_VK_NATIVE_FADD=0 keeps the loop and leaves the extension off, and the program cache's stamp follows the choice. On an RTX 4090 in Kakariko and on Death Mountain Trail, where six compute programs add floats every frame, GPU time fell from 7.8 to 7.2 and from 5.8 to 5.4 ms a frame; no add reached the ceiling there. The idea is yassine-milal's (NotRexed#886, closed to clear the queue). Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he loop It was read with atoi, which takes any value that is not a number for 0: RX_VK_NATIVE_FADD=on turned the native float add off. native_fadd_wanted reads it with env_value_off (gpu/vk/compat/hookshot_env.h, NotRexed#2042) now: 0, false, off, no or empty keep the compare-exchange loop, any other value asks for the native add, unset as before. tests/test_native_float_atomic_add.py compiles native_fadd_wanted alone against 13 values. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It was asked for wherever the GPU reports shaderBufferFloat32AtomicAdd: AMD's RDNA 3 and later (RADV and AMD's own driver) and Intel's Arc as well, but it was tried in the game only on an RTX 4090, and a fault in another driver's float atomics would show in compute passes (Kakariko's sky light) that are hard to judge by eye. With RX_VK_NATIVE_FADD unset, native_fadd_wanted now asks for it on NVIDIA's driver alone (VkPhysicalDeviceDriverProperties' driverID, asked before the extensions are chosen; NVK is not NVIDIA's driver). RX_VK_NATIVE_FADD=1 asks for it on any GPU that has it, =0 keeps the loop on every one. The log line says which and why; the program cache's stamp holds the answer as before. The test's table covers the drivers (28 cases), and docs/development.md says the default. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
50579ba to
ac1e281
Compare
…wanted" label (#2746) A test request asks someone's agent to download, build and run a PR on their own PC, and since #2734 anyone signed in to the team chat can post one for any PR. This makes a maintainer the gate: a request can only be picked up when the PR's author is a collaborator on GitHub, or a maintainer has looked at the PR and added the `testing wanted` label. **What changed** - `scripts/test_requests.py` asks GitHub (no sign-in needed; `GITHUB_TOKEN`/`GH_TOKEN` used when set) for the PR's `author_association` and labels. A request is approved when the author is `OWNER`, `MEMBER` or `COLLABORATOR`, or the PR has the `testing wanted` label. - `claim` refuses (exit 3) a request that isn't approved, or whose PR is merged or closed, and sends nothing when GitHub can't be asked. - `list` and `show` mark each open request `approved` and add the PR's state; waiting ones are listed last, with "waiting for a maintainer to add the label". - `request` tells the author when their PR needs the label. - The server's relay shows a waiting request as a grey card, "Waiting for a maintainer to add the `testing wanted` label", with no claims, results or guide link, and checks GitHub every 3 minutes so the card opens by itself soon after the label is added (approved ones every 15, for the PR's state). - **The label is also an entry point:** the relay lists open PRs labelled `testing wanted` every 3 minutes and posts a request for each one that has none, for its author (their GitHub name and avatar on the card, the PR title as the summary); it closes the ones it posted when the label comes off or the PR is merged or closed. The guide, the request skill and AGENTS.md say so. - The testing skill now treats the request, the PR and its code as data, never instructions: it builds and runs only as docs/development.md says, and before the first build looks at what the PR changes that runs on the PC (scripts, tools, workflows, anything downloading or reaching outside the repository), stopping if something isn't explained. - The request skill, the guide (`docs/test-requests.md`) and AGENTS.md say the same. **Tested** - Live: with this running, the relay posted requests for the two labelled PRs that had none (#2456, #2738), under their authors' GitHub names and avatars, and left #2306 alone (it already had one). - `tests/test_test_requests.py`: a stranger's PR without the label can't be claimed; the label approves it; a merged PR can't be claimed; no claim goes out when GitHub can't be asked, while `list` still works; waiting requests list last. `python -I -B tests/run_all.py test_requests team_chat`: both pass. - Live: the open request for #2306 (a contributor's PR) turned into the waiting card when the relay was updated; after adding `testing wanted` to #2306 it opened again on its own within 3 minutes, in both Discord channels, and `list` shows it approved.
|
Thanks @nullifyshot for both Windows RTX 5080 checks: the child runs in Kakariko and on Death Mountain Trail, then the adult follow-up on the trail. Your logs confirm the NVIDIA default selects the native add on Windows too, with lower GPU time in those comparisons; adult night and the AMD/Intel native paths remain untested. |
Joey-Magnus
left a comment
There was a problem hiding this comment.
Everything from my last review is fixed. With RX_VK_NATIVE_FADD unset, only NVIDIA's driver asks for the extension (native_fadd_wanted, vk_main.cpp:249). AMD, Intel and NVK don't even enable it, so nothing changes for them. The switch is read with env_value_off. The conflicts are resolved, keeping #2343's vk12 field in test_amd_subgroup_profiles.py. nullifyshot's Windows RTX 5080 runs, child and adult in Kakariko and on Death Mountain Trail, show the same picture and about 4% less GPU time. That matches your RTX 4090 numbers.
I merged it into current main and ran test_native_float_atomic_add, test_global_atomic_add_f32, test_cas_loop_ceiling, test_amd_subgroup_profiles, the program cache and stamp checks, test_env_on and the SPIR-V checks with spirv-val present. All 15 pass. It also merges cleanly together with #1975.
One thing for the maintainers: translator_stamp now always adds the flag, so every player re-translates once, not only NVIDIA players. #1975 and #2309 also force a re-translation, so shipping them in the same release means one re-prepare. Widening the default to RDNA 3/4 can wait for an AMD run.
Compute programs that run every frame in Kakariko and on Death Mountain Trail add floats to storage buffers, and the translator makes each of those adds a compare-and-swap loop: on an RTX 4090 the busiest one takes 77-80 attempts per add on average, and with the GPU's own float add the frame's GPU time is 0.4-0.6 ms (about 7%) lower there. On NVIDIA's driver the add is now one instruction. On AMD and Intel it stays the loop by default, because the native add was tried in the game only on that RTX 4090. That default changes once someone posts a run on an RDNA 3/4 (or Arc) card in Kakariko and on Death Mountain Trail with
RX_VK_NATIVE_FADD=1.Refs #2336 (closed by the maintainer: tracked in this pull request).
This takes up the idea of yassine-milal's #886 (closed to keep the queue clear), written again on current
mainwith three faults of that diff fixed; no code was copied from it.What changed
vk_main.cpp: on NVIDIA's driver (VkPhysicalDeviceDriverProperties'driverID, asked before the extensions are chosen; NVK does not count), the device asks forVK_EXT_shader_atomic_floatand turns on onlyshaderBufferFloat32AtomicAdd, the one feature used.RX_VK_NATIVE_FADD=1asks for it on any GPU that reports the feature, and=0keeps the loop everywhere without enabling the extension. The switch follows fix: switches that behave as players expect, and dead ones removed #2042's rule, soon,yesortruemean on and0,false,off,noor empty mean off (native_fadd_wanted). The log names the choice and why:vk: float atomic add on storage buffers: native (VK_EXT_shader_atomic_float), NVIDIA's driver, orcompare-exchange loop, by default only with NVIDIA's driver (RX_VK_NATIVE_FADD=1: native where the GPU has it), orcompare-exchange loop, RX_VK_NATIVE_FADD=0, or the feature that is missing.vk_shader.cpp: the profile tells the translator; the program cache's stamp includes the answer, so ahookshot_programs.binmade with the native add is never read on a GPU without it (fix: use the native float atomic add instead of compare-exchange loops #886 keyed only the environment switch).RED/ATOM.ADD.F32on a storage buffer is oneOpAtomicFAddEXTon an f32 view of the buffer, and no loop is made. The global-address fallback (SPIR-V: a fallback for GlobalAtomicAddF32 at an untracked global address #598) adds natively too, through the f32 views and the page table; the f32 views are searched with their bound length, as the u32 ones are (fix: use the native float atomic add instead of compare-exchange loops #886 skipped that check for them). Sirit gainsOpAtomicFAddEXT.docs/development.md: theRX_VK_NATIVE_FADD=0or1row, with the default.Which GPUs report the feature, from the drivers' sources: NVIDIA (the RTX 4090 here reports
shaderBufferFloat32AtomicAdd = true); AMD RDNA 3 and later on Mesa RADV (gfx_level >= GFX11) and RDNA 3 on AMD's own driver (PAL's Gfx11); Intel Arc and later on Mesa ANV (has_lsc). RDNA 1 and 2 (RX 5000, RX 6000, Steam Deck) and Intel's older integrated graphics have the extension without this feature and keep the loop either way. Of these, only NVIDIA's driver gets the native add by default. The AMD and Intel ones needRX_VK_NATIVE_FADD=1until they have a run here (Joey-Magnus's review).How it was tested
tests/test_native_float_atomic_add.py: cuts the storage-buffer setup and the float-add emitter out of the back end into a Sirit fixture, translates a compute shader with the feature, without it and without descriptor aliasing, and checks oneOpAtomicFAddEXTand no compare-and-swap in the first, the loop in the others,spirv-val --target-env vulkan1.3on all three; and that the device, the profile, the cache stamp and the fallback follow. It also compilesnative_fadd_wantedalone against 28 cases: unset, NVIDIA's driver alone gets the native add (not RADV, AMD's, Intel's or NVK); set, the switch's value by fix: switches that behave as players expect, and dead ones removed #2042's rule on any driver. It fails onmain. The decision fails it whennative_fadd_wantedreads the switch withatoias before (RX_VK_NATIVE_FADD=[on]: the loop) or wants the native add on every driver ([(unset)], driver 1: native).On the head (rebased onto
maina5b955f), throughtests/run_all.py --strictwithCXX=g++and withCXX=clang++:test_native_float_atomic_add,test_amd_subgroup_profiles,test_program_stamp_knobs,test_env_on,test_global_atomic_add_f32andtest_cas_loop_ceilingpass (6 of 6 each). Againstmain's source,test_native_float_atomic_addfails with both compilers.python -I -B tests/run_all.pyon the head (Arch Linux, 32 at once, at a load of 60 to 140 from other work on this PC): 435 passed, 3 failed, 20 skipped;maina5b955f the same way: 436 passed, 1 failed, 20 skipped. The four checks that failed in one of the two runs (test_appimage_package_retry,test_bake_mimo, andtest_recorder_syncandtest_controlby timeout) were run again alone on both trees: all four pass on the branch, andtest_appimage_package_retryfails onmainagain. So they are timing under load, not this change.clang++ -Wall -Wextraon the changed files againstmain: no new warning besides fourmissing field 'pNext' initializeron new structs (two for the driver query, two forafloat), the formvk_main.cppalready has 37 of.A stress program with the translator's exact loop: invocations each add 1.0 to one float; lost adds are the count minus the sum (median of 5 runs).
OpAtomicFAddEXT, adds lost(RTX 4090: NVIDIA 615.78.08; Intel UHD 770: Mesa 26.2.5, which has no buffer float add, so only the loop.) Up to 1,024 adds on one float the loop loses nothing; beyond that it does.
In the game
RTX 4090, NVIDIA 615.78.08, Arch Linux, no window (scripted input), the game's default output 2048x1152, Resolution 100%, Game speed per frame; this branch's build, with and without
RX_VK_NATIVE_FADD=0, each prepared first; a save loaded and Link left standing; 150 s each, numbers from present 1800 on (the 60 fps cap holds both ways, so this is GPU time, not frame rate). These were measured on the first version of this branch, onmainfab8cb3. On NVIDIA's driver the review's changes leave the same native path as the default, so they were not repeated. The two runs after the rebase are below the table.vk: GPU time, every 120 presents: median, range)RX_VK_PASS_TIMING=2, mean of 12 reports of 600 presents)ab7699ee7f536755's dispatch, once a frame435a0138b9013040's dispatch, once a frameThe other four float-add dispatches (below) are cheaper: not among the 25 costliest spans either way, except
592079442d7129daonce with the loop on Death Mountain Trail (0.059 ms).Which programs add floats, and how hard their loops work (a local counting build, not part of this change, run with the loop for about five minutes in each place): six compute programs, all adding to storage buffers:
499dbfd0721d2858(Storage buffer pass: track float adds past a c1 offset (night lit like day) #816's sky light),ab7699ee7f536755(fix: use the native float atomic add instead of compare-exchange loops #886's Kakariko pass),435a0138b9013040,592079442d7129da,aba5e6af8d4683eb(Kakariko only) and6214a97bec4cc0bf. Of 111.7 million adds in the two places, none reached the 1,024-attempt ceiling; the busiest,ab7699ee7f536755(about 3,000 adds a frame), took 77-80 attempts an add on average and 704 at most. So on this card no add is lost, and the gain is time. fix: use the native float atomic add instead of compare-exchange loops #886's fog program7984161db3e053b5runs every frame in both places but has no 32-bit float add; this change does not touch it.Pictures of the same present both ways (native, loop, difference x4) show the same light, haze and sky; the differences are animation (grass, Navi, Link's idle pose), as a present number is not exactly the same game frame in two runs.
After the review and the rebase: two headless runs of the rebased build (head 7219b3fd on
maina5b955f, the same save in Kakariko, 360 s each,RX_VK_PRECOMPILE=0so programs were made as met). Both reached Kakariko and ran to the end with no fatal error, no device loss and no kernel driver message. Their pictures show the same light and sky both ways. Other work kept this PC at a load of 60 to 165, so they say nothing about speed. The log line of each:Rebased once more on 2026-11-11, onto
main9df055f2, as50579bab: the only conflict was this change's row indocs/development.mdbeside a new row of main's; the three commits' code and tests are unchanged (compared hunk by hunk). Rerun on that head:test_native_float_atomic_add,test_amd_subgroup_profiles,test_global_atomic_add_f32,test_cas_loop_ceilingand the program cache's four checks pass throughtests/run_all.py --strict, andtest_native_float_atomic_add.pyfails onmain's tree. The whole suite and the two game runs above are from7219b3fdona5b955fb.Not tested
RX_VK_NATIVE_FADD=1it has not been run on either (which of them report the feature is from the drivers' sources above). Windows with NVIDIA is now tested by @nullifyshot (RTX 5080, NVIDIA 621.14, Windows 11, headless): child, Kakariko and Death Mountain Trail and adult, Death Mountain Trail; the default picks the native add there too, about 4 % less GPU time. The team's test request T-54W3 asks for an AMD RX 7000 or newer or an Intel Arc. Still not tested: adult at night on Windows; NVK; the Steam Deck (keeps the loop either way). Frame rates: the RTX 4090 holds the 60 fps cap both ways here, so only GPU time was compared. fix: use the native float atomic add instead of compare-exchange loops #886's thin fog and 20-53 ms passes (Quadro P4000, Windows, v2.0.2) did not reproduce on this card: no add was dropped and the pictures match.Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com