-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement custom mip map generation & raise maximum texture atlas size to 4096x4096 #5508
Conversation
The only apprehension I have here is our usage of
Before merging, this should be tested on every platform we have access to, especially lower powered ones. |
Would be good to have a test scene to test this precise scenario (which uploads a new texture each test step). |
With a focus on the spikes above average: 1024 With 8192 With 8192 With 8192 Without Pretty safe to say there is overhead which we need to address, but a path forward is probably very simple. I'd want to see similar testing done on an M1 GPU, as @smoogipoo has mentioned overheads with texture uploads. Diff to disable mipmapping: diff --git a/osu.Framework/Graphics/Textures/TextureAtlas_BackingAtlasTexture.cs b/osu.Framework/Graphics/Textures/TextureAtlas_BackingAtlasTexture.cs
index 6233a9950..abdd41544 100644
--- a/osu.Framework/Graphics/Textures/TextureAtlas_BackingAtlasTexture.cs
+++ b/osu.Framework/Graphics/Textures/TextureAtlas_BackingAtlasTexture.cs
@@ -32,7 +32,7 @@ private class BackingAtlasTexture : Texture
private static readonly Rgba32 initialisation_colour = default;
public BackingAtlasTexture(IRenderer renderer, int width, int height, bool manualMipmaps, TextureFilteringMode filteringMode = TextureFilteringMode.Linear, int padding = 0)
- : this(renderer.CreateTexture(width, height, manualMipmaps, filteringMode, initialisationColour: initialisation_colour))
+ : this(renderer.CreateTexture(width, height, true, TextureFilteringMode.Linear, initialisationColour: initialisation_colour))
{
this.padding = padding;
atlasBounds = new RectangleI(0, 0, Width, Height);
Testing can be done using #5517. Note that the screenshots above were running one upload per test step, while I've changed the test to run per 50ms, so the spikes will be more frequent on that branch. Can be adjusted as required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires further consideration
Performance wise, the new changes bring this back in line with |
Should be good for review now -- I'm done making implementation changes. |
86df7dc
to
7bb974d
Compare
Is memory usage not a concern here? |
If it's an issue we can drop back down to 4096x feasibly. But going forward we would be moving towards using a single atlas game-wide. I am currently working on combining the main atlas with the font store one, so that will reduce the startup overhead to just one atlas texture. For gameplay, I'm not sure what is making the atlases but it will need further investigation. The eventual plan is to add support for evicting textures from atlases so we can potentially have one shared global atlas across the whole game.
Can you show frame graphs of this happening, and provide hardware specs? |
That sounds good and would probably solve that issue completely once implemented.
My CPU is an intel i5 7600K (4 core ~4GHz), and my GPU a nvidia gtx 1060 (6GB). Here's a video showing the first map start: lazer.map.lag.textureatlas.mp4 |
@Morilli thanks for testing! Could you pull the latest commit and see if the lag spike got better? |
Indeed, this is much better! The tiny lag remaining is basically unnoticable now 👍 |
Also see #5521 which will reduce the memory by some factory. Gameplay requires a bit of further consideration, but on an initial check we can probably remove one of the two atlases there as well. |
From a memory overhead angle, we may want to consider bumping this up to 4096x4096 to start with (67mb per atlas texture vs 268mb for 8192x). At least until we have atlas eviction support and a better algorithm to allow for sharing a single atlas game-wide. For example, right now during gameplay there are 4-5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flags look a bit darker than usual, and resizing the game down to 640x480 makes them even darker.
Normal size:
640x480:
(notice the colour of the flag in the leaderboard card)
That being said, I haven't dwelt into the ends of the mipmap generation logic but overall it looks pretty interesting, and can see it eventually turning into its own class instantiated in Renderer
and exposed to textures via a GenerateMipmap
method that accepts a list of quads. But that can definitely be left for a follow-up.
// Initialize texture to solid color | ||
int frameBuffer = GL.GenFramebuffer(); | ||
GL.BindFramebuffer(FramebufferTarget.Framebuffer, frameBuffer); | ||
GL.FramebufferTexture2D(FramebufferTarget.Framebuffer, FramebufferAttachment.ColorAttachment0, TextureTarget2d.Texture2D, TextureId, level); | ||
Renderer.Clear(new ClearInfo(initialisationColour)); | ||
GL.DeleteFramebuffer(frameBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like scissoring and blending mask (technically colour write mask) should be reset here as well, otherwise I believe they will affect the initialisation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scissoring, depth, and colour write mask are reset internally to Renderer.Clear()
. Am I missing something here? Blending is not reset (probably bad), and depth still performs a depth test (probably bad) but those don't seem to have any effect in resolving the issue I pointed out in #5508 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scissoring and depth test sure, but colour write mask? I can't seem to find any line that resets that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about that - blend mask isn't set. Doesn't seem to have any effect though (disabled FTB).
@Tom94 since you know this best, can you give the flag issue above a quick check and provide your thoughts? I hope to get to reviewing this in the next few days. |
I had revisited this PR in efforts to improve performance on mobile devices (iOS specifically), and spent a good few hours now to get it to fully work on Veldrid. I've only tested this on Metal, checking other backends would be appreciated. |
@smoogipoo I can't reproduce what you're seeing above, maybe because I'm testing on M1. |
This comment was marked as resolved.
This comment was marked as resolved.
Turns out D3D11 doesn't like using the same texture as a sampling resource and a render target at the same time, so I've separated them for now. This makes me wonder whether we can replace all of this with a compute shader at some point, but I haven't looked into whether it can sample from and write to different mipmap levels of the texture (it may be feasible with read-write @EVAST9919 would appreciate confirming whether it works on your end now. |
@peppy need veldrid package update with ppy/veldrid#12 included for iOS to work (got broken with my latest changes above). |
Yep, all good now. |
Legacy-GL: This has broken some storyboards. Here's one, where random text will be black: https://osu.ppy.sh/beatmapsets/499488#osu/1073964. Legacy GL: My texture corruption issue still hasn't been fixed, but I haven't been able to reproduce any real world issues. Veldrid-VK: |
This should not have increased memory usage in any meaningful way (at most 50mb or so). I can't reproduce. Please continue dsucssion in the new issue you opened rather than taking a shot in the dark at the cause. It's quite possibly not related. |
Undoes #2585
Larger texture atlases drastically reduce the number of draw calls by avoiding texture switching.
The previous limit of 1024x1024 is not even enough to hold the latin font (as used in the font tests), let alone cjk. In osu! song select, the number draw calls is reduced from ~900 to ~400.
Large texture atlases were previously not used, because their mipmap generation (then based on
GL.GenerateMipmap
) was slow, causing stutters. This PR introduces a custom mipmap generation pipeline that only touches the regions of the texture atlas that are being updated, hence circumventing stutters.