erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions

Revert "Campaign V slice V4a" - it lost world multisampling

Browse source 
This reverts ceec3bc4. Two independent reasons, either sufficient.

The rendering regression. The slice deleted TextRenderGlStateScope, which
saved GL_MULTISAMPLE and GL_SAMPLE_ALPHA_TO_COVERAGE on entry, disabled them
for the text pass, and restored them on exit (TextRenderGlStateScope.cs:111-112
and 153-154 at the parent commit). Its replacement bakes that state into the
text pipeline but nothing restores it, and GlGpuPassEncoder.Dispose does not
either. Every world renderer is still raw GL at this point in the campaign, so
from the first UI frame onward the world drew with multisampling disabled.

The offline pixel gate caught it: 1,791 of 563,200 compared pixels differed,
0.318% against a 0.001 threshold. The commit message attributed this to
wall-clock-driven ambient animation shifting phase, and committed through the
failure. That explanation does not survive its own control: capturing twice at
the reverted-to commit differs by 19 pixels and twice at the slice's own commit
by 8, while base-versus-head differs by 1,791 - a 224x gap that no shared-noise
source explains. An amplified difference image settles it visually: the changed
pixels are the silhouette edges of every tree, building and rock, with terrain
interiors, water and the entire UI untouched. That is the signature of losing
edge antialiasing, not of animated sprites.

This is the exact failure mode two existing memory notes already warn about -
a mid-frame renderer must set every GL state it uses rather than inherit it,
and issue #52's lesson that a rendering migration must audit per-pass GL state
before declaring itself done.

The scope. The brief was three small leaf renderers plus additive frame-
lifecycle wiring, roughly ten files. The commit changed 334 files with 3,665
insertions and 3,845 deletions, including 323 public-to-internal visibility
conversions across the App assembly, 55 test files, two retired conformance
tests, and a self-described temporary escape hatch for bridging raw-GL viewport
textures. Even without the regression, that is not separable into the part
worth keeping and the part worth dropping.

Reverting rather than patching because the good work here - the RHI frame
lifecycle wiring and a genuine render-state-cache staleness fix - is small
enough to redo cleanly against a tightened spec, while untangling it from 300+
files of unrelated churn is not.

Post-revert: Release build clean, App suite back to 3,843 passed / 3 skipped,
offline pixel gate passing at 19 differing pixels.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-07-27 18:27:52 +02:00
parent ceec3bc440
commit 9aaf97e785
334 changed files with 3841 additions and 3661 deletions
Download patch file Download diff file Expand all files Collapse all files

84
src/AcDream.App/Rendering/Gpu/Gl/GlGpuDevice.cs
View file

@ -190,26 +190,9 @@ internal sealed class GlGpuDevice : IGpuDevice
string fragmentPath = Path.Combine(_shadersDirectory, $"{description.Shaders.Name}.frag");
string vertexSource = File.ReadAllText(vertexPath);
string fragmentSource = File.ReadAllText(fragmentPath);
// Every RHI-created pipeline gets the same common.glsl preamble the
// legacy Shader(gl, vert, frag, includeCommonPreamble: true) call
// sites already opt into (mesh_modern, terrain_modern): the set-0
// binding-9 texture-table buffer + ACDREAM_TEXTURE_HANDLE macro that
// slice V2 introduced. A pipeline whose shaders never reference the
// macro simply carries an unused SSBO declaration — harmless — so
// there is no reason for callers to opt in per pipeline.
string commonSource = CommonPreambleSource;
vertexSource = Shader.InjectPreamble(vertexSource, commonSource);
fragmentSource = Shader.InjectPreamble(fragmentSource, commonSource);
return new GlGpuPipeline(_gl, Retirement, description, vertexSource, fragmentSource);
}
private string? _commonPreambleSource;
private string CommonPreambleSource => _commonPreambleSource ??=
File.ReadAllText(Path.Combine(_shadersDirectory, "common.glsl"));
public IGpuRenderTarget CreateRenderTarget(in GpuRenderTargetDescription description)
{
ThrowIfDisposed();
@ -232,33 +215,6 @@ internal sealed class GlGpuDevice : IGpuDevice
return new GpuTextureSlot(slot);
}
/// <summary>
/// Registers an externally-created, externally-owned GL texture name
/// (a private-viewport FBO colour attachment) into the same texture table
/// <see cref="RegisterTexture"/> uses. Not part of <see cref="IGpuDevice"/>
/// — <c>PrivateEntityViewportRenderer</c>/<c>PaperdollViewportRenderer</c>
/// still allocate their own FBO textures directly on <c>GL</c> pre-V4g, so
/// their retained-UI consumers (<see cref="UiViewport"/>) need a bridge
/// that does not require the caller to own an <see cref="IGpuTexture"/>.
/// The caller keeps deleting the GL name itself; this only ever writes
/// the bindless handle into the table and returns the slot. Deleted with
/// the GL backend once slice V4g moves those renderers onto
/// <see cref="IGpuRenderTarget"/> and they can call
/// <see cref="RegisterTexture"/> like every other consumer.
/// </summary>
internal GpuTextureSlot RegisterExternalColorTexture(uint glTextureName, IGpuSampler sampler)
{
ThrowIfDisposed();
ArgumentNullException.ThrowIfNull(sampler);
if (sampler is not GlGpuSampler glSampler)
throw new ArgumentException("The GL backend can only register a GL sampler.", nameof(sampler));
uint slot = _textureSlotAllocator.Allocate();
ulong handle = _bindless.GetResidentHandle(glTextureName, glSampler.GlName);
WriteHandle(slot, handle);
return new GpuTextureSlot(slot);
}
public void ReleaseTextureSlot(GpuTextureSlot slot)
{
ThrowIfDisposed();
@ -283,17 +239,6 @@ internal sealed class GlGpuDevice : IGpuDevice
long serial = ++_nextSerial;
int slot = _frameFlights.CurrentSlot;
_ringStates[slot].Reset();
// Campaign V slice V4a: during the migration, every still-legacy
// renderer (WbDrawDispatcher, terrain, particles, EnvCellRenderer, ...)
// mutates real GL program/blend/depth/cull state directly, outside this
// device's ApplyRenderState. _renderState has no way to observe those
// calls, so its cached "current" snapshot goes stale the moment any of
// them runs between two RHI binds. Resetting once per frame is the same
// defensive move BeginPass already makes after a forced clear (see the
// comment there) — it costs one redundant state application on the
// frame's first BindPipeline, in exchange for never diffing against a
// baseline the driver has since moved past.
_renderState.Reset();
return new GlGpuFrame(this, slot, serial);
}
@ -338,17 +283,6 @@ internal sealed class GlGpuDevice : IGpuDevice
_textureHandleTable.AsSpan(tableStart, tableEnd - tableStart));
_textureTableBuffer.Upload((long)tableStart * sizeof(ulong), bytes);
}
// Bind the device's own table to binding 9 immediately before every
// RHI-issued draw. Storage-buffer bindings are global GL context
// state, and the still-unmigrated legacy renderers (WbDrawDispatcher,
// EnvCellRenderer, TerrainModernRenderer, ParticleRenderer) rebind
// their OWN interim per-renderer table to the same binding right
// before their own draws (slice V2's documented pattern) — so this
// device must re-claim binding 9 before ITS draws too, or an RHI
// draw issued after a legacy draw in the same frame would read the
// wrong table.
_gl.BindBufferBase(GLEnum.ShaderStorageBuffer, GpuBindingModel.StorageTextureTable, _textureTableBuffer.GlName);
}
internal void BeginPass(GpuPassDescription description)
@ -461,24 +395,6 @@ internal sealed class GlGpuDevice : IGpuDevice
{
_gl.ColorMask(desired.ColorWrite, desired.ColorWrite, desired.ColorWrite, desired.ColorWrite);
}
if (changes.Multisample)
{
// Vulkan bakes multisample rasterization into the pipeline via its
// declared sample count — there is no separate enable bit. The GL
// backend mirrors that: GL_MULTISAMPLE tracks
// GpuPipelineDescription.SampleCount rather than being left at
// whatever the previous pass happened to leave it. This matters
// because the default framebuffer can itself be multisampled
// (QualitySettings MSAA): a single-sample UI pipeline binding
// while GL_MULTISAMPLE is still enabled from an earlier world
// pass converts each glyph's soft alpha edge into dithered MSAA
// coverage instead of a clean alpha blend (the pre-RHI
// TextRenderer explicitly disabled it for the same reason).
if (desired.Multisample)
_gl.Enable(EnableCap.Multisample);
else
_gl.Disable(EnableCap.Multisample);
}
GLHelpers.ThrowOnResourceError(_gl, "apply GL render state");
}

3
src/AcDream.App/Rendering/Gpu/Gl/GlGpuPassEncoder.cs
View file

@ -51,8 +51,7 @@ internal sealed class GlGpuPassEncoder : IGpuPassEncoder
description.Cull,
description.FrontFace,
description.AlphaToCoverage,
description.ColorWrite,
Multisample: description.SampleCount > 1);
description.ColorWrite);
_device.ApplyRenderState(desired);
_gl.BindVertexArray(p.GlVertexArray);

13
src/AcDream.App/Rendering/Gpu/Gl/GlRenderStateCache.cs
View file

@ -15,8 +15,7 @@ internal readonly record struct GlRenderStateSnapshot(
GpuCullMode Cull,
GpuFrontFace FrontFace,
bool AlphaToCoverage,
bool ColorWrite,
bool Multisample);
bool ColorWrite);
/// <summary>Which GL state calls are needed to move from the previous snapshot to the new one.</summary>
internal readonly record struct GlRenderStateChanges(
@ -28,15 +27,14 @@ internal readonly record struct GlRenderStateChanges(
bool Cull,
bool FrontFace,
bool AlphaToCoverage,
bool ColorWrite,
bool Multisample)
bool ColorWrite)
{
public bool AnyChange =>
Program || Blend || DepthTest || DepthWrite || DepthCompare
|| Cull || FrontFace || AlphaToCoverage || ColorWrite || Multisample;
|| Cull || FrontFace || AlphaToCoverage || ColorWrite;
/// <summary>Every dimension reported changed — used for the first apply after a reset.</summary>
internal static GlRenderStateChanges All { get; } = new(true, true, true, true, true, true, true, true, true, true);
internal static GlRenderStateChanges All { get; } = new(true, true, true, true, true, true, true, true, true);
}
/// <summary>
@ -70,8 +68,7 @@ internal sealed class GlRenderStateCache
p.Cull != desired.Cull,
p.FrontFace != desired.FrontFace,
p.AlphaToCoverage != desired.AlphaToCoverage,
p.ColorWrite != desired.ColorWrite,
p.Multisample != desired.Multisample);
p.ColorWrite != desired.ColorWrite);
}
/// <summary>Discards the cached baseline — the next <see cref="Apply"/> reports every dimension changed.</summary>