Skip to content
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

[UAP] Alternative RenderLoop #5352

Closed
wants to merge 2 commits into from

Conversation

nkast
Copy link
Contributor

@nkast nkast commented Dec 7, 2016

Use Idle for Tick(). Let Dispatcher process all events (input/resize/rotate) before rendering.

Fixes mouse, keyboard lag [UAP] touch, rotation lag & freeze (WP8.1).
#4897, #5300, #3948, #3589, #3316, #4105(?).

@nkast
Copy link
Contributor Author

nkast commented Dec 7, 2016

RunIdleAsync creates a new IdleDispatchedHandlerArgs object on every
frame. I changed it to RunAsync(Low), keyboard/mouse events are still good!

We still need RunIdleAsync to start the loop, otherwise we get a blank
screen. Probably StartRunLoop() needs to be called after
xaml/SwapChainPanel OnCreated event.

I am going make a few tests and submit a fix for WP8.1/W8.1 as well.

GamePad.Back = false;
};
CoreWindow.GetForCurrentThread().Dispatcher.RunAsync( CoreDispatcherPriority.Low, OnRenderFrame);
});
Copy link
Member

Choose a reason for hiding this comment

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

This looks sort of weird... why are you dispatching once with RunIdleAsync to just dispatch again RunAsync(Low)?

Seems easier to just do:

public override void StartRunLoop() 
{
   OnRenderFrame();
}

This will do one tick to kick things off then register itself for the next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's too early to call RenderFrame(). And I had to dispatch with RunIdleAsync the first time so that XAML initialize & create events arrives before OnRenderFrame();.

@tomspilman
Copy link
Member

Anyone had time to test this on a game that suffered from the input lag?

Also should check if it affects your overall game performance.

@SirDragonClaw
Copy link

SirDragonClaw commented Dec 8, 2016

Anyone had time to test this on a game that suffered from the input lag?

@tomspilman I have just given it a go. My games keyboard inputs went from 2-20 seconds of lag to 10-100 seconds of lag with this change.

Performance was not effected in my limited testing.

It is not what I expected, perhaps someone else can give it a go?

@tomspilman
Copy link
Member

My games keyboard inputs went from 2-20 seconds of lag to 10-100 seconds of lag with this change.

Are you saying the keyboard input lag is worse? 100 seconds of lag worse?

@SirDragonClaw
Copy link

SirDragonClaw commented Dec 8, 2016

Are you saying the keyboard input lag is worse? 100 seconds of lag worse?

Yeah exactly, much much worse.

@KonajuGames
Copy link
Contributor

My games keyboard inputs went from 2-20 seconds of lag to 10-100 seconds of lag with this change.

Almost two minutes of lag?

@SirDragonClaw
Copy link

Almost two minutes of lag?

If I constantly move the mouse around yes. I actually think that I can delay the input forever if I keep moving the mouse.

@nkast
Copy link
Contributor Author

nkast commented Dec 8, 2016

I have just given it a go. My games keyboard inputs went from 2-20 seconds of lag to 10-100 seconds of lag with this change.

That's strange, maybe I did something wrong when I moved to Idle->Low priority.
I rewrote that, now I schedule next frame on Idle priority when the queue is not empty, otherwise on Low priority to eliminate the garbage of RunIdleAsync. In theory the app will consume all queued events within a frame or two. I tested it on desktop/phone/emulator, the same for WP8.1/W8.1 project and didn't notice any lag.

@SirDragonClaw
Copy link

I will give it another go tomorrow if I get a chance. Perhaps I stuffed something up somehow.

@nkast
Copy link
Contributor Author

nkast commented Dec 8, 2016

@Danthekilla BTW, are you using FixedTimeStep? and how is your frame rate?
I tried to emulate most senarios with my test.

@SirDragonClaw
Copy link

We are not using a fixed timestep, our framerate is from 50 to 60. (capped from vsync)

@BrianPeek
Copy link
Contributor

I'm tired and likely missing something in what I'm reading...

Isn't this change going to only render a new frame if the message queue is empty? So, if I'm moving the mouse (generating messages) or some such, wouldn't this not ever render a frame?

I'm sleepy and shouldn't be commenting when sleepy. :)

@tomspilman
Copy link
Member

Isn't this change going to only render a new frame if the message queue is empty?

Yeah that sounds like what is happening.

What we are looking for is something like the old school windows message loops:

while(true)
{
    // Process all pending messages until none remain.
    while(PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
    {
        TranslateMessage(&msg);
        DispatchMessage(&msg);
    }

    // No pending messages so update/draw.
    game->Tick();
}

With these loops input events wouldn't get missed and the game would draw as fast as possible in old Win32/DirectX games.

The key difference here being the tick didn't depend on its own event... which can be infinitely delayed by the message system. My guess is that new events are continuously queued even during message processing and all end up ahead of our idle/low event... so we never get to run.

@nkast
Copy link
Contributor Author

nkast commented Dec 10, 2016

@BrianPeek

Isn't this change going to only render a new frame if the message queue is empty?
So, if I'm moving the mouse (generating messages) or some such, wouldn't this not ever render a frame?

If messages arrive faster than we can process them, then yes. In practice I didn't find any problems with mouse input neither with touch input on a phone. For store apps we enqueue mouse/touch events and process them later in the game loop before Update(), so it doesn't keep the thread for too long. Certainly it wouldn't hurt to remove any unnecessary overhead (#5158).

What you said is exactly what is happening right now to input events. CompositionTarget.Rendering event fires 60 times per second with high priority. On every tick we hold the UIThread for 16.6ms, if Draw is slow and miss Vsync it's +16.6ms. By the time Tick returns, a new Rendering event is already dispatched in the queue, Only few events can make it through this narrow window. Mouse events start to pile up with ever-growing lag and push back any keyboard events (lower priority than mouse).

For example, when I tested this PR on WP8.1/WP10 with Low priority for Tick, the rotation event never come through! Normally I have to use Idle every time but I suppose a one-frame lag rotation is acceptable to avoid garbage.

@nkast
Copy link
Contributor Author

nkast commented Dec 10, 2016

@tomspilman

What we are looking for is something like the old school windows message loops:

In store apps there is the Dispatcher.ProcessEvents(ProcessAllIfPresent), but that's for CoreApplication.Run loops, you can't call it from within an event. In XAML, the UIThead runs YOU :simple_smile:

The PR tries to emulate exactly that. Another way to see it is like the Invalidate/WM_PAINT event in MFC/WinForms.

The key difference here being the tick didn't depend on its own event... which can be infinitely delayed by the message system.

I don't think there's a difference between the two. You can stack in while(PeekMessage()) as well if you have a slow Mouse handler.

@nkast
Copy link
Contributor Author

nkast commented Dec 10, 2016

In a quick search for mouse polling rate, someone mention the value 125Hz (8ms) but it's really up to the driver and you could set it up to 1000Hz, although my logitech SetPoint doesn't have any setting for that.

In @Danthekilla's comparison of the frequency mouse events, it is clear that there is a separation of 4ms between mouse events. So I'd say that a rate of 250Hz is typical nowadays.

In the worst case scenario of 1000Hz you have at least 1ms/event to keep the queue empty. There would be 16 events in the queue between ticks.

In a pragmatic scenario of 250Hz you have at least 4ms/event to keep the queue empty. There would be 4 events in the queue between ticks.

{
MetroGameWindow.Instance.Tick();
};
var dispatcher = CoreWindow.GetForCurrentThread().Dispatcher;
Copy link
Contributor Author

@nkast nkast Dec 17, 2016

Choose a reason for hiding this comment

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

We can make MetroGameWindow._coreWindow internal and get it directly via MetroGameWindow.Instance._coreWindow instead of calling CoreWindow.GetForCurrentThread() on every frame.

@tomspilman
Copy link
Member

@nkast @BrianPeek - So where does this leave us here?

I like the idea of preserving the existing threading model where the game can be expected to be running from the UI thread. It is what we've been doing since Windows 8 (because that is how the DirectX samples did it) and has worked. If we can make this work under UWP that is preferred.

However if we have to change tactics here because there is no other good alternative then lets figure out how to do it with the least amount of pain to the developer.

@SirDragonClaw
Copy link

As a semi related thing I should mention that I have gotten the non xaml version of UWP monogame working quite well now, all input works correctly and it now correctly handles screen resizing and device resets.

However as of today we have decided that it won't work for us as the loss of xaml (mainly for advertising) is too big a loss. I will be giving this pull request another go tomorrow to see if it fixes up the delayed keyboard input. Because we need to move back to a xaml system sadly.

@tomspilman
Copy link
Member

@Danthekilla - So how often do you interact with the XAML side of the app from the MonoGame code? How painful would it be if you needed to use some thread synchronization and/or dispatch to communicate between these sides?

@SirDragonClaw
Copy link

@tomspilman Not often. We mainly use it for ad overlays which are just told to show or hide for the most part. And we also use it for our in app currency coin system that we are planning to try out. We were just going to rewrite that UI in monogame but since we haven't found any decent ad providers that don't require XAML we will still with XAML for that UI too.

Doing some simple thread synchronization and dispatching would be absolutely fine.

@nkast
Copy link
Contributor Author

nkast commented Dec 19, 2016

I 'am confident about that one, just waiting @Danthekilla's confirmation.

One issue would be that low priority events like Keyboard will lag by one frame because of the workaround to eliminate garbage. (I wonder if IdleDispatchedHandlerArgs generates garbage or being a WinRT uses ref counting and doesn't trigger the GC). I can solve this by calling .RunIdleAsync(OnRenderFrame) immediately when .ShouldYield() is true, instead of calling Tick() and schedule an idle event for next time.

@SirDragonClaw
Copy link

@nkast
Thanks for having a go of the sample.

The FPS counter was copied from a old XNA sample I didn't notice that that had any errors. When I would set the draw count low I would get 144FPS reported consistently on my 144hz screen. You can set the sample span to one second, I just had it set to 100ms for testing.

So I didn't see the new method affecting actual frame rate in any way.

That's odd, my FPS went from 90 to 30 (or as low as 5fps under mouse load) when moving to ARL I will have to check my FPS counter again because I don't think it could possibly be out by that much.

Can you test it yourself on another system?

We have tested on 2 systems, (however they do have the same hardware) I will test on a few other devices when I get back to work.

For as long as the message queue is full we don't get the .Closing event.

So this means the message queue is full indefinitely? My closing events never get triggered.

@nkast
Copy link
Contributor Author

nkast commented Dec 23, 2016

@Danthekilla

Setting FpsSampleSpan to 1 sec would give better results but it also drifts with every reset.
I removed the reset and sample the fps whenever the stopwatch increased by one second.

For as long as the message queue is full we don't get the .Closing event.

So this means the message queue is full indefinitely? My closing events never get triggered.

While the message queue is what keeps the app alive I was wrong about the closing event. The closing event never fires on store apps. I need a way other than UAPGameWindow.Instance.IsExiting to terminate the loop. I will setup a check around Suspend/Resume events.

@BrianPeek
Copy link
Contributor

Hey @Danthekilla, I did a quick and dirty test of the XAML project using a simple multithreaded renderer + CoreIndependentInputSource vs. everything running on the main thread. As with my other test, this is not not complete/tested, and shouldn't be used for anything real, but I'd love it if you had time to test it out with your project and report back results. You can find the project/branch here.

Thanks!

@nkast nkast force-pushed the tncAlternativeRenderLoop branch 2 times, most recently from 21b5ad9 to ec19545 Compare December 24, 2016 12:43
@jakepoz
Copy link
Contributor

jakepoz commented Dec 27, 2016

Just had a chance to test these builds, both nkasts and BrianPeek's solutions. I used Danthekilla's test project, but I removed the artificial CPU load (comment out the ThreadPool.RunAsync(BackgroundTask) call)

With both solutions the input lag is gone for me, and they also close down cleanly for me when exiting. If I switch to Monogame 3.5, there is terrible input lag, even without the artificial CPU load.

The artificial load pegs my CPU to 100% on all cores, and basically every application acts weird in this case. Even then, I didn't see input lag where events were delayed by seconds, just a general sluggishness of response.

BTW, we did have a $1,000 USD bounty open with this bug: https://www.bountysource.com/issues/7309032-wp8-1-multitouch-slowdown and we are of course happy to pay it out once this fix gets accepted.

@SirDragonClaw
Copy link

@BrianPeek THIS WORKS!

Zero input lag, moderately stable framerate, full XAML support!

I have had to make some fairly large changes to monogame to dispatch in quite a few places where it was accessing the corewindow and fix up some issues where it was accessing the currentthreads window but I have a build that doesn't have the input bug and has working XAML now.

WorkItemHandler workItemHandler = action => 
{ 
	while(true) 
	{ 
		UAPGameWindow.Instance.Tick(); 
		GamePad.Back = false;	
	} 
}; 
ThreadPool.RunAsync(workItemHandler, WorkItemPriority.High, WorkItemOptions.TimeSliced); 

This single change in StartRunLoop() fixes the input issue 100% of the time. You just have to remember to dispatch everything when you need to talk to the UI thread.

@nkast
Copy link
Contributor Author

nkast commented Jan 10, 2017

@BrianPeek, @Danthekilla
Since I already tested separate render thread some time ago, here are a few thing you should look at:

  • Resize/Rotate events sync. Try to resize the window for a few seconds and a sharpDX exception will be thrown. Probably you must check all other events and how they interact with the GraphicsDevice.

  • MG and/or the user must dispatch calls to XAML & all .GetForCurrentThread()/.GetForCurrentView() methods.

  • coreIndependentInputSource bug with touch devices. I assume that every decent game disables the touch feedback. When using coreIndependentInputSource the touch feedback is always on, causing frame drops on weak tablets with low fillrate because the OS does one extra fullscreen composition. You can check this on a tablet or the emulator. We can't avoid coreIndependentInputSource, otherwise touch events from the main thread get serious lag (seen on WP8.1). CoreIndependentInputSource is the real blocking issue here. Unless MS fixes it across the board (W8.1/WP8.1 -> W10) I wouldn't use it in my games. And for all I know, they haven't even acknowledge it as a bug yet.

BTW, @BrianPeek , @tomspilman Can we get a pure CoreApplication project template along with XAML?
@Danthekilla were there any issues you solved with CoreApplication and resize?

Also @Danthekilla, did you check this PR on other machines? Any clue what is causing the keyboard lag?

@BrianPeek
Copy link
Contributor

BrianPeek commented Jan 10, 2017

I definitely didn't explore resizing/rotate/etc. or any other events with the threaded thing I threw together. I just setup the threads to see if it would help before going fully down that path. We'll need to do that if this is the solution that makes its way forward.

I wrote a CoreApplication template which others tested, but it also needs to be vetted more closely. That's in a branch here. Again, needs to be finished, but the basics are there.

@KonajuGames
Copy link
Contributor

I believe the threaded approach is the correct way to progress here. Yes, anything that interacts with XAML has to be invoked, and the resize issues would need to be fixed.

@BrianPeek Is the CoreIndependentInputSource touch feedback visualization bug referenced above by @nkast known by the Windows team? If you know who to prod gently...

@jakepoz
Copy link
Contributor

jakepoz commented Jan 15, 2017

I was doing some testing today regarding the Alternate Render Loop within real-life games that we have developed and published. First, I tested the latest changes on this PR, and it did fully resolve the input lag bug. (Our games are not CPU intensive and Dan's repro project involves maxing out all CPUs in order to see input lag issues)

One thing I noticed was related to the XAML based Ad SDK we use from Microsoft. When I built our game with a develop branch from a few days ago, the input lag is there, but if there is a heavy-duty "rich media" ad playing with animations, it tends to play at a low framerate. The game itself occasionally stutters when an ad like this loads, but after that the game plays mostly fine. On the latest tncAlternativeRenderLoop branch, the input lag is fixed, but these types of ads render at 60fps, while the game itself stutters a lot.

The CoreApplication template is not good for developers who want to monetize with any standard ad SDK which all require XAML.

Does this point to a threaded approach like @KonajuGames is suggesting?

@theZMan
Copy link
Contributor

theZMan commented Jan 15, 2017

That would make sense... the alternate loop puts all of the game on a separate thread leaving the main windows to handle just input..

Your Ad SDK sees that the main windows is doing almost nothing and promptly sucks up all the CPU probably now starving the bg thread. We might want to make our background thread a higher priority - that might help it beat out the ad.

Is this on win 10 or phone?

Any way to run the ad SDK on a separate thread?

@jakepoz
Copy link
Contributor

jakepoz commented Jan 15, 2017

This is on a Win 10 UWP project for desktop. I don't think there is a way to run certain XAML controls on a separate thread.

@BrianPeek
Copy link
Contributor

The AlternateRenderLoop from @nkast doesn't do threads, so not sure @theZMan your thought is correct? My potential fix is the one with threads... @jakepoz did you try that? https://github.com/BrianPeek/MonoGame/tree/XamlThreads

@theZMan
Copy link
Contributor

theZMan commented Jan 16, 2017

@BrianPeek - yes I was confused - too many branches. Fingers crossed!

@jakepoz
Copy link
Contributor

jakepoz commented Jan 16, 2017

Just tested @BrianPeek 's XamlThreads solution, and both the input lag and XAML Ad SDK are working properly. I did have to adjust our project to dispatch the XAML stuff separately, but it was a small change.

Just as @nkast said though, it doesn't work with resizing the window. During the actual resize it will lag a lot and not actually process the resize events fast enough, and will also occasionally crash.

SharpDX.SharpDXException: HRESULT: [0x887A0005], Module: [SharpDX.DXGI], ApiCode: [DXGI_ERROR_DEVICE_REMOVED/DeviceRemoved], Message: The GPU device instance has been suspended. Use GetDeviceRemovedReason to determine the appropriate action.

   at SharpDX.Result.CheckError()
   at SharpDX.DXGI.SwapChain.ResizeBuffers(Int32 bufferCount, Int32 width, Int32 height, Format newFormat, SwapChainFlags swapChainFlags)
   at Microsoft.Xna.Framework.Graphics.GraphicsDevice.CreateSizeDependentResources()

@BrianPeek
Copy link
Contributor

@jakepoz, yes, my branch is unfinished and was just a PoC for threads. I need to go back and make sure we process events on the correct threads...

@nkast
Copy link
Contributor Author

nkast commented Jan 17, 2017

@jakepoz

On the latest tncAlternativeRenderLoop branch, the input lag is fixed, but these types of ads render at 60fps, while the game itself stutters a lot.

That's bad. Game loop runs at idle priority and ads take over the CPU. I guess it nullifies the advantage ARL has with XAML interoperability (no dispatching).

Does this point to a threaded approach like @KonajuGames is suggesting?

Yes, I am afraid that the only option to bring performance back when using XAML controls/animations.
For resizing for example we can use a _targetSize variable and resize the window before a frame starts if we need to. Exit event uses something similar (_shouldExit) and might work out of the box with a separate thread. We have to avoid locking because of performance and potential deadlocks.

Use Idle for Tick(). Let Dispatcher process all events
(input/resize/rotate) before rendering.

RunIdleAsync creates a new IdleDispatchedHandlerArgs object on every
frame, Use RunAsync + Low priority instead of Idle.
We still need RunIdleAsync to start the loop, otherwise we get a blank
screen. Probably StartRunLoop() needs to be called after
xaml/SwapChainPanel OnCreated event.
Use Idle for Tick(). Let Dispatcher process all events
(input/resize/rotate) before rendering.
@nkast
Copy link
Contributor Author

nkast commented Mar 14, 2017

Closed via #5520

@nkast nkast closed this Mar 14, 2017
@nkast nkast deleted the tncAlternativeRenderLoop branch March 25, 2017 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants