Add TemporarySpriteBatch by SnipUndercover · Pull Request #1096 · EverestAPI/Everest

SnipUndercover · 2026-03-24T00:57:14Z

Adds a helper TemporarySpriteBatch type to easily interrupt an existing SpriteBatch, optionally swap in a RenderTarget2D, and start a new SpriteBatch.
When disposed, the new SpriteBatch is ended, the old RenderTarget2D is restored and the previous SpriteBatch is resumed.

~~Also bumps the language version to C# 13, as this is necessary for ref structs to implement interfaces.~~
~~We already need .NET 9 to compile Everest, so we might as well make use of C# 13.~~

EDIT: Implicitly implementing IDisposable lets us go back to <LangVersion> 12, so the version bump isn't necessary. Ideally we should implement IDisposable but this will work for now, until we jump to .NET 10.

Everest already needs the .NET 9 SDK to compile, so we might as well utilize its features.

microlith57

initial comments, will review more thoroughly later!

microlith57 · 2026-03-24T01:06:51Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

+        [MaybeNull] out Effect customEffect,
+        out Matrix transformMatrix)
+    {
+        // life would be good if we could just access these directly...


query: why can't we? surely monomod has some way do do this right?

I am using DynamicData here.
It'd be great if we didn't have to resort to DynamicData or reflection and just access fields directly.

yes that's what i mean, i don't know if monomodlinkto does what we want but if not i would expect a similar thing exists
i guess in the worst case we can always write the body of this method in il

The JIT would still check access permissions, unless we [IgnoreAccessChecksTo("FNA")], but I don't feel like this is a good idea.

Everest already applies patches to FNA, I think it wouldn't be too crazy to do this, especially since this api is by definition going to be called on a hot path. I could see it being useful for mods to be able to access the current sprite batch state in general as well. (Doesn't Everest already have a different place (Parallax?) where it gets this information as well?)

yea, definitely agree. it's well within everest's remit to do this. if we just ignore access checks from everest to fna (while still requiring mods to do that manually themselves if applicable), that should be fine right?

I'm wondering what sort of patches would be necessary here to avoid having to use DynamicData without breaking mods who might be reflecting on it...

I think adding get-only props to SpriteBatch would be fine, and wouldn't conflict with reflection.

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatchBuilder.cs

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

Saplonily · 2026-03-24T05:20:28Z

Since we've already on .net 8, what about enable null context for this file instead of using these attributes?

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

JaThePlayer · 2026-03-24T13:52:28Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

+        [MaybeNull] out Effect customEffect,
+        out Matrix transformMatrix)
+    {
+        // life would be good if we could just access these directly...


Everest already applies patches to FNA, I think it wouldn't be too crazy to do this, especially since this api is by definition going to be called on a hot path. I could see it being useful for mods to be able to access the current sprite batch state in general as well. (Doesn't Everest already have a different place (Parallax?) where it gets this information as well?)

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

JaThePlayer · 2026-03-24T18:26:54Z

The [MaybeNull] annotations used wreak havoc on actual Nullable Reference Types: (this is from Frost Helper, which has NRT enabled, so the target parameter is specified to be not-null here)

EDIT:
It gets worse:

The attribute seemingly should be [AllowNull] instead, or the files could instead have NRT enabled via #nullable enable if we're going with nullability annotations anyway.

SnipUndercover · 2026-03-29T22:50:45Z

The [MaybeNull] annotations used wreak havoc on actual Nullable Reference Types: (this is from Frost Helper, which has NRT enabled, so the target parameter is specified to be not-null here)

I missed this, huh. I guess we can go with #nullable enable and using NRT here.

microlith57

still uncomfortable with the DynamicData so this isn't an approval yet. but good progress, i like the ArrayPool!

microlith57 · 2026-03-30T07:08:37Z

Celeste.Mod.mm/Celeste.Mod.mm.csproj

    <AssemblyName>Celeste.Mod.mm</AssemblyName>
    <RootNamespace>Celeste</RootNamespace>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
-    <LangVersion>11</LangVersion>


nitpick/query: did you mean to remove this entirely? why not explicitly specify 12? i guess if it's the default that's fine but seems strange nevertheless

I thought we were using C#11 since we're in .NET 8 but apparently not.
Not setting the language version explicitly makes it default to the latest one at the time of the release of the version specified by the TFM. In this case it would be C#12.
Though, since several other PRs have suggested bumping the language version, I think we should move that change to a separate PR.

microlith57 · 2026-03-30T07:10:12Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

+        [MaybeNull] out Effect customEffect,
+        out Matrix transformMatrix)
+    {
+        // life would be good if we could just access these directly...


yea, definitely agree. it's well within everest's remit to do this. if we just ignore access checks from everest to fna (while still requiring mods to do that manually themselves if applicable), that should be fine right?

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

JaThePlayer · 2026-03-30T16:59:04Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

+    /// <summary>
+    ///   The <see cref="SpriteSortMode"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly SpriteSortMode CurrentSortMode;
+
+    /// <summary>
+    ///   The <see cref="BlendState"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly BlendState CurrentBlendState;
+
+    /// <summary>
+    ///   The <see cref="SamplerState"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly SamplerState CurrentSamplerState;
+
+    /// <summary>
+    ///   The <see cref="DepthStencilState"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly DepthStencilState CurrentDepthStencilState;
+
+    /// <summary>
+    ///   The <see cref="RasterizerState"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly RasterizerState CurrentRasterizerState;
+
+    /// <summary>
+    ///   The custom <see cref="Effect"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly Effect? CurrentCustomEffect;
+
+    /// <summary>
+    ///   The transformation <see cref="Matrix"/> of this <see cref="TemporarySpriteBatch"/>.
+    /// </summary>
+    public readonly Matrix CurrentTransformMatrix;
+
+    /// <summary>
+    ///   The <see cref="RenderTarget2D"/> swapped in for the duration of this <see cref="TemporarySpriteBatch"/>
+    ///   or <c>null</c> to draw to the screen if <see cref="HasRenderTarget"/> is <c>true</c>; else always <c>null</c>.
+    /// </summary>
+    public readonly RenderTarget2D? CurrentRenderTarget;
+


These Current... fields seem a bit questionable, since they're only used in the ctor and bloat the already-large struct even more.
The fields could be useful as a way to pass rendering info around various methods, but the Builder seems better to use for this purpose (since the code receiving a builder can use it to restart/change the batch etc).
If Everest exposes a public way of retrieving this info from the SpriteBatch directly, these become less important as well.

JaThePlayer · 2026-03-30T17:06:10Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatchBuilder.cs

+///   <b>Note: Restarting a spritebatch flushes it to the GPU.</b>
+///   While the cost is not that significant, it's best to avoid restarting the spritebatch too often per frame.
+/// </remarks>
+public sealed class TemporarySpriteBatchBuilder


Maybe worth making a record (struct?) to allow easily making a copy of the builder (to later change some of its settings)? Or maybe just a Clone() method instead.

If this doesn't become a struct, would be good to make the TemporarySpriteBatch ctor public for cases where caching the builder is not reasonable. (In which case, the ctor should probably have default values for all parameters so it's easy to pick and choose named parameters as needed).

JaThePlayer · 2026-03-30T17:09:36Z

Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs

+        [MaybeNull] out Effect customEffect,
+        out Matrix transformMatrix)
+    {
+        // life would be good if we could just access these directly...


I think adding get-only props to SpriteBatch would be fine, and wouldn't conflict with reflection.

SnipUndercover added 2 commits March 24, 2026 00:55

Bump .csproj language version

b0a2e51

Everest already needs the .NET 9 SDK to compile, so we might as well utilize its features.

Implement temporary SpriteBatches

48a7332

microlith57 reviewed Mar 24, 2026

View reviewed changes

maddie480-bot added the 1: review needed This PR needs 2 approvals to be merged (bot-managed) label Mar 24, 2026

JaThePlayer reviewed Mar 24, 2026

View reviewed changes

SnipUndercover added 6 commits March 30, 2026 00:46

Adjust GPU upload notes

07aa520

Make TemporarySpriteBatch.Dispose() idempotent

b0d6ee8

Implicitly implement IDisposable

5ffbae5

Roll back LangVersion to C# 12 (.NET 8 default)

6d2a866

Avoid allocations by using an ArrayPool

5ccdca3

Remove redundant bool properties

43132ee

SnipUndercover requested review from JaThePlayer and microlith57 March 29, 2026 22:49

SnipUndercover added 2 commits March 30, 2026 00:55

Use Nullable Reference Types

1c04847

Fix possible null dereference in Dispose

b6d9d04

SnipUndercover changed the title ~~Add TemporarySpriteBatch & bump LangVersion to C# 13~~ Add TemporarySpriteBatch Mar 30, 2026

microlith57 reviewed Mar 30, 2026

View reviewed changes

JaThePlayer reviewed Mar 30, 2026

View reviewed changes

Conversation

SnipUndercover commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

microlith57 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Saplonily commented Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JaThePlayer commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SnipUndercover commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

microlith57 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SnipUndercover commented Mar 24, 2026 •

edited

Loading

JaThePlayer commented Mar 24, 2026 •

edited

Loading

SnipUndercover commented Mar 29, 2026 •

edited

Loading