Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Celeste.Mod.mm/Celeste.Mod.mm.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
<AssemblyName>Celeste.Mod.mm</AssemblyName>
<RootNamespace>Celeste</RootNamespace>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<LangVersion>11</LangVersion>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<ShouldIncludeNativeLua>false</ShouldIncludeNativeLua>
</PropertyGroup>

Expand Down
244 changes: 244 additions & 0 deletions Celeste.Mod.mm/Mod/Helpers/TemporarySpriteBatch.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
#nullable enable

using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using Monocle;
using MonoMod.Utils;
using System.Buffers;

namespace Celeste.Mod.Helpers;

/// <summary>
/// A temporary <see cref="SpriteBatch"/>.
/// </summary>
/// <remarks>
/// When constructed, the current <see cref="SpriteBatch"/> properties and <see cref="RenderTarget2D"/> are preserved.
Comment thread
microlith57 marked this conversation as resolved.
/// Then, the <see cref="SpriteBatch"/> is ended, the <see cref="RenderTarget2D"/> is swapped if necessary,
/// and finally the <see cref="SpriteBatch"/> is restarted with the new properties.<br/>
/// When disposed, the previous <see cref="SpriteBatch"/> properties and <see cref="RenderTarget2D"/> are restored.
/// <br/>
/// Useful when interrupting a <see cref="SpriteBatch"/> mid-render to, for example, render a specific entity to a
/// temporary <see cref="RenderTarget2D"/> while applying a custom shader, all while preserving the previous
/// configuration.<br/>
/// <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>
/// <seealso cref="TemporarySpriteBatchBuilder"/>
public ref struct TemporarySpriteBatch
{
// we don't really expect the RenderTargetBinding[] array size to exceed 1,
// so a max of 16 seems reasonable
private static readonly ArrayPool<RenderTargetBinding> _renderTargetPool
= ArrayPool<RenderTargetBinding>.Create(0x10, 50);
Comment thread
microlith57 marked this conversation as resolved.

/// <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;

Comment on lines +34 to +74
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// The <see cref="SpriteSortMode"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly SpriteSortMode PreviousSortMode;

/// <summary>
/// The <see cref="BlendState"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly BlendState PreviousBlendState;

/// <summary>
/// The <see cref="SamplerState"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly SamplerState PreviousSamplerState;

/// <summary>
/// The <see cref="DepthStencilState"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly DepthStencilState PreviousDepthStencilState;

/// <summary>
/// The <see cref="RasterizerState"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly RasterizerState PreviousRasterizerState;

/// <summary>
/// The custom <see cref="Effect"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly Effect? PreviousCustomEffect;

/// <summary>
/// The transformation <see cref="Matrix"/> that was used prior to the start of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly Matrix PreviousTransformMatrix;

/// <summary>
/// The <see cref="RenderTargetBinding"/>s that were used prior to the start 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 RenderTargetBinding[]? PreviousRenderTargets;


/// <summary>
/// Whether a <see cref="RenderTarget2D"/> was swapped in for the duration of this
/// <see cref="TemporarySpriteBatch"/>.
/// </summary>
public readonly bool HasRenderTarget;

/// <summary>
/// Whether this <see cref="TemporarySpriteBatch"/> is still active.
/// </summary>
public bool Active { get; private set; }


/// <summary>
/// Create and immediately begin a new <see cref="TemporarySpriteBatch"/>.
/// </summary>
/// <seealso cref="TemporarySpriteBatchBuilder"/>
internal TemporarySpriteBatch(
SpriteSortMode? sortMode,
BlendState? blendState,
SamplerState? samplerState,
DepthStencilState? depthStencilState,
RasterizerState? rasterizerState,
Effect? customEffect,
Matrix? transformMatrix,
RenderTarget2D? renderTarget,
bool hasCustomEffect,
bool hasRenderTarget)
{
GetSpriteBatchFields(
out PreviousSortMode,
out PreviousBlendState,
out PreviousSamplerState,
out PreviousDepthStencilState,
out PreviousRasterizerState,
out PreviousCustomEffect,
out PreviousTransformMatrix);

CurrentSortMode = sortMode ?? PreviousSortMode;
CurrentBlendState = blendState ?? PreviousBlendState;
CurrentSamplerState = samplerState ?? PreviousSamplerState;
CurrentDepthStencilState = depthStencilState ?? PreviousDepthStencilState;
CurrentRasterizerState = rasterizerState ?? PreviousRasterizerState;
CurrentCustomEffect = hasCustomEffect ? customEffect : PreviousCustomEffect;
CurrentTransformMatrix = transformMatrix ?? PreviousTransformMatrix;

HasRenderTarget = hasRenderTarget;

GraphicsDevice graphicsDevice = Engine.Graphics.GraphicsDevice;
if (hasRenderTarget)
{
int renderTargetCount = graphicsDevice.GetRenderTargetsNoAllocEXT(null);
if (renderTargetCount > 0)
{
PreviousRenderTargets = _renderTargetPool.Rent(renderTargetCount);
graphicsDevice.GetRenderTargetsNoAllocEXT(PreviousRenderTargets);
}
Comment thread
microlith57 marked this conversation as resolved.
CurrentRenderTarget = renderTarget;
}

Active = true;
Draw.SpriteBatch.End();
if (hasRenderTarget)
Engine.Graphics.GraphicsDevice.SetRenderTarget(CurrentRenderTarget);
Draw.SpriteBatch.Begin(
CurrentSortMode,
CurrentBlendState,
CurrentSamplerState,
CurrentDepthStencilState,
CurrentRasterizerState,
CurrentCustomEffect,
CurrentTransformMatrix);
}

/// <summary>
/// End this <see cref="TemporarySpriteBatch"/>, restore the previous render targets if necessary, and restore
/// the previous <see cref="SpriteBatch"/> properties.
/// </summary>
public void Dispose()
{
if (!Active)
return;

Active = false;
Draw.SpriteBatch.End();
if (HasRenderTarget)
{
Engine.Graphics.GraphicsDevice.SetRenderTargets(PreviousRenderTargets);
_renderTargetPool.Return(PreviousRenderTargets!, clearArray: true);
}
Draw.SpriteBatch.Begin(
PreviousSortMode,
PreviousBlendState,
PreviousSamplerState,
PreviousDepthStencilState,
PreviousRasterizerState,
PreviousCustomEffect,
PreviousTransformMatrix);
}

private static void GetSpriteBatchFields(
out SpriteSortMode sortMode,
out BlendState blendState,
out SamplerState samplerState,
out DepthStencilState depthStencilState,
out RasterizerState rasterizerState,
out Effect? customEffect,
out Matrix transformMatrix)
{
// life would be good if we could just access these directly...
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


DynamicData dynData = DynamicData.For(Draw.SpriteBatch);
sortMode = dynData.Get<SpriteSortMode>("sortMode");
blendState = dynData.Get<BlendState>("blendState")!;
samplerState = dynData.Get<SamplerState>("samplerState")!;
depthStencilState = dynData.Get<DepthStencilState>("depthStencilState")!;
rasterizerState = dynData.Get<RasterizerState>("rasterizerState")!;
customEffect = dynData.Get<Effect>("customEffect");
transformMatrix = dynData.Get<Matrix>("transformMatrix");
}
}
Loading
Loading