diff --git a/TEXTURES.md b/TEXTURES.md index 1f9448d..e38e5df 100644 --- a/TEXTURES.md +++ b/TEXTURES.md @@ -213,3 +213,123 @@ Open questions a reviewer should weigh in on: texture set should already be bounded by what is on screen rather than by the whole conversation — worth confirming that invariant holds before relying on it to keep atlas/bind-group churn small. + +## Review, 2026-09-04 + +A second pass over the file above against the code, done before anything +is implemented. Iris's worry going in: a bind group per texture means a +draw call per image, and she wants this as efficient as it can be. + +### What checked out + +Every code reference above is accurate as of this commit: the 100,000 / +1,000 limits, the one-time pipeline, the `rsc_group` rebuild on every +`Push`/`Set`/`Free`, and the `Patch` exclusion. The device request that +asks for the three features is `iris/src/default/render.rs:96`, which the +text above does not name. egui-wgpu and Vello are described correctly. + +### The emulator refusal is a wgpu-hal gap, now located + +The file guessed "a likely instance-version negotiation gap." It is +narrower than that and it is in wgpu-hal, not the emulator. wgpu-hal +28.0.0 (`src/vulkan/adapter.rs:1618`) only queries +`PhysicalDeviceDescriptorIndexingFeaturesEXT` **when the device advertises +the `VK_EXT_descriptor_indexing` extension string**. A Vulkan 1.2+ driver +that has descriptor indexing as core need not list the extension, and +lavapipe at 1.3 evidently does not, so wgpu never asks and reports the +features absent, which is why `ash` sees seven `true`s and wgpu sees none. +The properties query beside it (line 1486) correctly accepts +`device_api_version >= 1.2 || extension`; the features query does not. +wgpu-hal 30.0.1 in the local registry has the same asymmetry (lines +1872 and 2036). Worth an upstream issue, but not a reason to keep the +design: on real phones the gate that matters is stricter still. + +**wgpu's `TEXTURE_BINDING_ARRAY` needs six sub-features, not one** +(`adapter.rs:160-177`): non-uniform indexing *and* update-after-bind for +sampled images, storage images and storage buffers, all together, because +wgpu marks every array-bearing descriptor set update-after-bind. So Arm's +"the extension is supported on Valhall" is necessary but not sufficient; +a driver with sampled-image indexing and without storage-buffer +update-after-bind is refused too. That widens the excluded set beyond +what the Arm quote suggests and strengthens the conclusion. + +### A live bug in the current code, found on the way + +`GpuTextures::update` (`core/src/render/texture.rs:33`) implements +"a patch must not report changed" as `changed = false`, unconditionally, +which also **cancels a `Push` earlier in the same batch**. That ordering is +exactly what opening a new atlas page produces: `GlyphAtlas::allocate` +pushes the page and `insert` patches it in the same frame, so the bind +group is not rebuilt and the new page's view is not bound until some +unrelated texture change happens to rebuild it. It is hidden today only +because the masks path also sets `changed`. The fix is one line +(`changed |= !matches!(update, Patch)` in spirit); it should go in with +the redesign since that code is being replaced, and it is recorded here +so it is not rediscovered. + +### In-layer draw order is already undefined + +Relevant to any batching redesign: `Primitives::apply_free` +(`core/src/render/primitive.rs:147`) uses `swap_remove`, so the instance +order within a layer is permuted whenever anything is freed. Overlap order +inside one layer is therefore not something the renderer promises today; +ordering is done with layers. That means grouping a layer's draws by +texture, or drawing a layer's images after its rects and glyphs, loses +nothing that currently exists. It should be written down as an invariant +when the redesign lands, because the new code will depend on it. + +### On "a draw call per image" + +Two corrections to the worry. First, it is a draw per *distinct texture per +layer*, not per image primitive: every glyph quad in a layer shares the +atlas and stays one instanced draw, and a thumbnail atlas would do the same +for previews. Second, the count is bounded by what is on screen, which I3's +virtualised transcript already bounds, and a mobile GPU is not draw-call +bound at tens of draws per frame; egui ships exactly this on Android. What +does cost is per-frame *bind group creation* and per-frame *sorting*, and +the current code already creates a `primitive_group` bind group every time +a layer updates (`render/mod.rs:103`), so one more per new image is not a +regression in kind. + +### Recommended shape (proposal, for Iris to accept or change) + +Aimed at the fewest moving parts that need no feature beyond Vulkan 1.0: + +1. **Atlas pages become layers of one `texture_2d_array`**, not separate + textures. Every page is already `PAGE`x`PAGE` RGBA8, which is the one + constraint an array texture imposes. A layer index is an ordinary + sampling operand in WGSL and needs no indexing feature, so `GLYPH` + (and any future atlased-image primitive) carries a layer instead of a + `view_idx` and all of a layer's text stays **one draw**. This answers + the open question above about keeping a small binding array: no. Cost + of opening a page: recreate the array with one more layer and + `copy_texture_to_texture` the old ones, GPU-side, no readback; grow + with headroom (double) so it is rare. wgpu's default + `max_texture_array_layers` is 256, at 4 MB each, so the cap is memory + rather than the API. +2. **Every standalone image is its own texture with its own bind group**, + and its instances live in a **separate per-layer instance list**, not + the main one. Then the main instance buffer never contains an image, + there is nothing to sort, no handle remapping beyond what + `apply_free` already does, and each image is `draw(0..4, k..k+1)` with + its bind group set first. Group 2's layout becomes `{atlas array, + one image texture, sampler, masks}`; the main draw binds a 1x1 null + image in the image slot, each image draw binds its own. One pipeline, + one shader, one layout. +3. **No thumbnail atlas in the first version.** With images on their own + textures, the threshold and eviction questions above disappear: an + image is freed when the row that owns its `TextureHandle` scrolls out. + Add an image atlas only if a measured screen shows enough small images + to matter, which a transcript rarely does. +4. **Drop the three features and the two `max_binding_array_*` limits from + `src/default/render.rs`**, and the `UiLimits` counts with them. +5. **Sampling is `NonFiltering` today** (`render/mod.rs:290,299`), so a + downscaled attachment will alias. Either request a filtering sampler + for the image slot or downscale on the CPU before upload; decide when + the image widget is touched, not as part of this. + +What this costs against the file's original recommendation: `Textures` +needs to know an image from a page (two kinds of handle, or a kind on +`TextureHandle`), and `Primitives` gets a second instance list per layer. +What it saves: the sort, the size threshold, the eviction policy, and any +per-page bind group switch.