Allow Texture2D.GetData On Non UI Threads

[squarebananas]

Currently Texture2D.GetData can only be run from the UI thread for DesktopGL projects. WindowsDX / UWP projects allow Texture2D.GetData from other threads.

This pull request uses the BlockOnUIThread approach (already used for Texture construction, Buffer GetData, etc) to add this functionality.

This code change is just swapping EnsureUIThread to the following, but the PR shows 50 additions and 49 deletions due to the indent change:

Threading.BlockOnUIThread(() =>
{
    // Existing Code
}

For demonstration purposes please see the TextureGetDataThread project within here:
https://github.com/squarebananas/MonoGameSamplesForIssues

[Jjagg]

The reason this throws on non-UI threads, is because people generally expect GetData to fill the array immediately. They might try to use the data immediately after the GetData call. That said, people of course might sometimes want to use GetData from a non-UI thread. But they should be made aware of the delay in that case.

I think maybe a better approach would be to:

• Add a GetDataUnsafe (not sure that's the best name for it) method that uses BlockOnUIThread.
• Keep EnsureUIThread in the normal GetData method, but have a parameter for custom error text. In this case letting users know about the OpenGL limitation and that they should use GetDataUnsafe to get data before the next frame, but not immediately.

We should look into inconsistencies with the buffer GetData methods; they should act in the same way.

[squarebananas]

Just to understand the issue more, the concern is that Texture2D.GetData will take up to 1 frame to complete?

In particular I was loading a heightmap in a level loading thread and needed GetData for the terrain construction. This had always worked fine in XNA, MonoGame WindowsDX and MonoGame UWP. It appeared BlockOnUIThread had been missed here for DesktopGL projects when compared to other GetData methods which is why I submitted the PR.

Should the other GetData methods not have used the BlockOnUIThread approach?

[Jjagg]

Just to understand the issue more, the concern is that Texture2D.GetData will take up to 1 frame to complete?

Yes, exactly.

Should the other GetData methods not have used the BlockOnUIThread approach?

In my opinion not. Anecdotally, I mostly see GetData used to fill an array that's used immediately after. Code like that will not work on OpenGL with BlockOnUIThread.

In particular I was loading a heightmap in a level loading thread and needed GetData for the terrain construction.

Yeah, this is exactly the kind of case for which it would be useful to still allow calling GetData on OpenGL off the UI thread.

I think EnsureUIThread with an alternative method that schedules the call on the UI thread seems like a good solution, but I'm not sure that's the best design, so alternative suggestions are welcome.

cc @harry-cpp @mrhelmut

[mrhelmut]

I'm not a fan of BlockOnUIThread in this context but I must admit that EnsureOnUIThread has a misleading behavior because it postpone the job to the next frame.

In my opinion, both are problematic, and I don't think there is a golden path because either mechanism will trigger different use-cases. Ultimately, I don't think that we want to block, because that would be very visible to players and unavoidable to developers, while postponing to the next frame is workable (though tricky to developers and currently undocumented).

Dunno if we could rework the OpenGL threading stuff, it's really misleading as it is but as far as I gave thoughts to it, I couldn't figure a better model than postponing to the next frame (and manually considering it in code). The biggest issue is that this behavior is undocumented and problems such as this one (or having unload/load sequences making two textures to coexist in memory while they shouldn't) can arise.

[squarebananas]

In my opinion not. Anecdotally, I mostly see GetData used to fill an array that's used immediately after. Code like that will not work on OpenGL with BlockOnUIThread.

The biggest issue is that this behavior is undocumented and problems such as this one (or having unload/load sequences making two textures to coexist in memory while they shouldn't) can arise.

Just reading these comments again, I'm still not sure I understand correctly. When BlockOnUIThread is used with GetData then running GetData on a non UI thread will not return an array until it has run on the UI thread. It doesn't return an array that gets populated later. The non UI thread is blocked until GetData is run on the UI thread. No changes to the game code should be necessary.

The only difference should be it runs slower. For example if it's a single texture for a heightmap loaded on a thread it will take up to one frame longer to run which is barely noticeable. Where it becomes noticeable is when GetData is run for a few hundred textures on a thread as it will add approximately 1 second to the thread's completion time for every 60 textures at 60 fps.

[Jjagg]

You are right. Sorry for confusing things. It seems I actually imprinted a wrong idea of BlockOnUIThread in my head 4 years ago, thinking it just scheduled the task and did not wait for it to complete (which I feel dumb about because the Block part is in the method name). Sooo, yeah the whole reason we have EnsureUIThread in the first place is me misunderstanding BlockOnUIThread 😓

So with that cleared up for me, I agree GetData should use BlockOnUIThread.

Like @mrhelmut said, it would be nice to document this behavior. I think perhaps we should have a doc page with OpenGL specifics/limitations. But this PR is great as is IMO :)

having unload/load sequences making two textures to coexist in memory while they shouldn't

I don't follow, when would this issue occur?

[stromkos]

WindowsDX / UWP projects allow Texture2D.GetData from other threads.

Historically, XNA had a single threaded model, therefore WindowsDX and all SharpDX derived platforms, do as well.

With the SDL platforms, there is a separate rendering thread (by necessity on some platforms like Android).

Where it becomes noticeable is when GetData is run for a few hundred textures on a thread as it will add approximately 1 second to the thread's completion time for every 60 textures at 60 fps.

The given approximation depends on the UI thread runtime vs the game thread runtime. Not to mention that a one second delay on the main thread in Windows will cause the close window due to unresponsive task box to appear if any click or key-press is made while the main thread is waiting.

There are too many XNA tutorials offering per pixel sprite collision detection that work fine when single-threaded, but quietly and silently break (slow down to a crawl) with this change.

The current behavior forces the programmer to consider the impact and react somewhat appropriately. Even moving the calls to Draw(), there is still no guarantee of data consistency across threads upon access from Update().

---

What if any effect does IsRunningSlowly have on a separate thread? Could this create a race condition?

---

I would suggest in 4.0, requiring all GetData() and SetData() calls must be batched.`;
Thus allowing for a single wait period for the threads to synchronize.

cc @harry-cpp @mrhelmut @Jjagg

[squarebananas]

Historically, XNA had a single threaded model, therefore WindowsDX and all SharpDX derived platforms, do as well.

In my experience XNA allowed GetData requests without issue from other threads (I used this extensively on Xbox 360 games).

Just to make it clear as stated above other GetData functions work on DesktopGL platforms (such as VertexBuffer.GetData), it's only Texture2D.GetData that has been missed.

This PR just adds the missing BlockOnUIThread approach already in use through out the DesktopGL project.

The given approximation depends on the UI thread runtime vs the game thread runtime. Not to mention that a one second delay on the main thread in Windows will cause the close window due to unresponsive task box to appear if any click or key-press is made while the main thread is waiting.

The UI thread will continue to run its updates each frame like normal while the thread is active. In the example the level loading thread will just take longer to complete (as stated one frame per GetData request longer). This already happens when loading textures in a loading thread and displaying a game loading animation on the UI thread.

What if any effect does IsRunningSlowly have on a separate thread?

IsRunningSlowly is based on the concept of a frame drop occuring on the UI thread. Other threads just do the work they are given regardless of whether the UI thread is updating/drawing so IsRunningSlowly isn't really related to that. (Also IsRunningSlowly currently doesn't work correctly #7571).

Edit: I think you mean what would happen if the frame was skipped when using a fixed time step and no draw occurred. That isn't an issue because pending threading actions occur outside of the Game.Tick. Even if that weren't the case a draw is forced if more than 0.5 seconds passes and update hasn't catched up.

MonoGame/MonoGame.Framework/Platform/SDL/SDLGamePlatform.cs

Lines 96 to 99 in 2850e2d

	SdlRunLoop();
	Game.Tick();
	Threading.Run();
	GraphicsDevice.DisposeContexts();

[stromkos]

Would it not be better to synchronize the game and UI threads to replicate the XNA single threaded model, with a proper implementation of IsRunningSlowly?

The goal of MonoGame versions < 4 is unity or at least parity with XNA.

[stromkos]

In my experience XNA allowed GetData requests without issue from other threads (I used this extensively on Xbox 360 games).

Yes, the main game thread was blocked on the call, as your change purposes.

The difference is in XNA all of your calls were attended to in a long single frame (other thread delay + game loop time + execution time)

With your change is that each and every call blocks the continuation of the Game thread for each call made.

My solutions: Either provide a batch calling method(4.0) or bind the threads together, make the UI thread wait on the game thread(Update) each step, like XNA, making locking and waiting unnecessary.

[squarebananas]

The purpose of the PR was just to add the missing BlockOnUIThread which is already used in 22 other similar situations. Currently without the PR it throws an exception.

There may be better approaches going forward but whatever changes are made would ideally need to be made to the other 22 instances of BlockOnUIThread. Presumably that would be an entirely separate issue/PR.

Also the Game thread shouldn't be blocked, only additionally created background threads (for example level loading threads). For DesktopGL the UI thread and Game thread are the same (which is why dragging the title bar freezes the game) unless I'm missing something?

[stromkos]

Also the Game thread shouldn't be blocked...

This depends on usage, multiple direct calls from Game1 as shown in tutorials for XNA, some of which either directly state or imply no performance penalty(as is the case in a single threaded model), would lead to a lock release delay cycle(s) in the multi-threaded model, thereby possibly degrading the main and/or rendering threads' performance.

unless I'm missing something?

No, I was not objecting to the PR, only suggesting possible better ways moving forward. Which I agree, that it needs to be in a separate PR. I was simply here commenting to plant the seed for later changes needed for all 23 instances in version 4, since the suggestions would break XNA compatibility in the API.

[squarebananas]

If the calls are directly from Game1 there is no additional locking as BlockOnUIThread will run the action directly. If the call is from an additionally created background thread then BlockOnUIThread blocks until the UI thread completes the work, the UI thread itself is not blocked.

This process already occurs when for example using Content.Load[Texture2D] or Content.Load[SpriteFont] in a loading thread for a loading screen.

[mrhelmut]

I would suggest in 4.0, requiring all GetData() and SetData() calls must be batched.`;

Considering newer APIs like Vulkan, D3D12, and consoles... this is likely what is going to happen. I don't see ourselves trying to go into complex command synchronisation stuff just for those operations (which would totally bottleneck such pipelines). Those API are not meant to wait on GPU operations to complete, and it is encouraged to have everything operating asynchronously. OpenGL is a different beast, it is meant to be entirely sequential and blocking, and is not friendly with threading at all (which is why those blocking were introduced in the first place). OpenGL is meant to disappear and at some point we will redirect DesktopGL users to the future DesktopVK implementation, which will likely assume GetData/SetData to be async.

I also believe that we should discourage the usage of those operations. They are way too often (unknowingly) abused for operations that don't need them or for problems which could be solved differently and more efficiently. There are use cases where they are useful and I'm not saying that we should remove them, but we should update the documentation to explain the nature of those operations and to think twice or thrice before using them. Maybe we should output a warning whenever they are used in debug builds.

We didn't even implemented them on consoles, and people seem okay with that because they end up finding better solutions in their absence.

I believe that we should be fine with blocking. That will expose to users how those operations can be an issue and will discourage them to use them. We could add a console output warning when running from a thread like "this operation is blocking the UI thread".

[squarebananas]

We could add a console output warning when running from a thread like "this operation is blocking the UI thread".

I could amend the PR to add console output warnings for all BlockOnUIThread calls although it's the non UI thread that is blocked not the UI thread itself.

Should I add these lines?
https://github.com/MonoGame/MonoGame/blob/158c0154ac18ed6102c65e24665c6a080ccb8ed2/MonoGame.Framework/Platform/Threading.cs
Line 141 Debug.WriteLine("Thread blocked, waiting for action to be completed by the UI thread");
Line 199 Debug.WriteLine("Pending action completed by the UI thread");

[mrhelmut]

Sounds good to me.

[squarebananas]

Sounds good to me.

Thanks, I've added those just.

[dellis1972]

These will be stripped out for release builds won't they? Which means someone using the NuGet packages will not see these messages. Trace.WriteLine might be a better option.

[tomspilman]

Not sure what is the best option here, but these are my concerns:

1. On release/shipping builds we don't want to slow down the game code with lots of debug checks that should not happen in a shipping game.

2. Since NuGet is our only delivery option how do we add debug/diagnostic checks that aid in development, but doesn't hurt when you ship? Are there Debug NuGet packages?

Note this may be a bigger discussion beyond the scope of this PR. But for games we cannot waste even a little time on debug checks that 99.99% never fire in a shipping game.

[squarebananas]

After a quick check just there are around ~25 other Debug.WriteLine instances with potentially useful information.

Maybe they should all go to a new MonoGame method where all such output messages can be rerouted at runtime. For Debug this could always route to Debug.WriteLine as default and for Release somewhere else only if Debugger.IsAttached is true.

User defined Streams could also be attached to get these messages through a custom Console app, localhost webpage, etc.

In addition by adding unique error codes for each type of message this could be used for custom filtering and disabling of error checking/reporting by error code or error category (audio, input, graphics, video errors).

[Mindfulplays]

Debug.WriteLine is fully optimized away (including the interpolated string) in non-DEBUG builds.

I added a GraphicsDebug.Blah in my private repo to streamline these. Maybe I can bring these in a separate PR?