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

Closing UI windows makes sound skip #1539

Closed
petchema opened this issue Sep 29, 2019 · 21 comments
Closed

Closing UI windows makes sound skip #1539

petchema opened this issue Sep 29, 2019 · 21 comments

Comments

@petchema
Copy link
Collaborator

Describe the bug
Using Resources.UnloadUnusedAssets() in UserInterfaceWindow.CloseWindow() makes sound skip or stutter for many people, even with no mods (hence minimal memory usage).

Forums: https://forums.dfworkshop.net/viewtopic.php?f=27&t=10&p=25780#p25780

To Reproduce
Listen to sound while closing any UI window.

Expected behavior
Music and sounds to play seamlessly while using UI

Desktop (please complete the following information):
Tested with 0.10.7 on Linux, but it can be noticed on many gameplay videos

Additional context
Resoures.UnloadUnusedAssets() is supposed to be called automatically by Unity when new scene is loaded, I guess explicit call was added because Daggerfall Unity doesn't load many scenes (beside very early during execution). But could it be called in places that would be equivalent to scene loading: loading game, fast travel, teleport, entering/leaving buildings?
Alternatively, I personally used a patch that leaves the call in UserInterfaceWindow.CloseWIndow() but throttles garbage collection so it doesn't happen more than once every 5 minutes, which is sufficient to prevent eventual long term memory usage runaways.
Distant Terrain mod also seems to call Resoures.UnloadUnusedAssets()

@petchema
Copy link
Collaborator Author

petchema commented Jan 1, 2020

Example / WIP:
master...petchema:load-scenes-gc

@Nystul-the-Magician
Copy link
Contributor

Nystul-the-Magician commented Mar 21, 2020

As far as I remember we put this in on purpose, we had a lot of ppl running out of memory and experience crashes without it because gc would not happen often enough

distant terrain definitely needs it, without it is basically crashing for most ppl after a few minutes because memory usage goes up and never goes down again

@petchema
Copy link
Collaborator Author

I'm not advocating removing those calls altogether, because they're probably required (mainly because DFU doesn't use LoadScene() much I suppose).
However calling it from UserInterfaceWindow.CloseWindow() doesn't seem like a good idea, because it produces a noticeable skip for many people. Once you noticed that issue, it becomes unbearable.
I think we should investigate other options, like the patch I linked above. (investigating again could also be a good idea because we changed the version of Unity we use, so memory management may have improved).

Distant Terrain also contains a call to Resources.UnloadUnusedAssets() when origin is moved, as long as it does it's not concerned by how frequently we call UnloadUnusedAssets from DFU itself.

@Nystul-the-Magician
Copy link
Contributor

alright, makes sense ;)

@petchema
Copy link
Collaborator Author

petchema commented Apr 7, 2020

Something that may help is the incremental garbage collection that recent versions of Unity now feature (assuming it's GC that is actually what takes time in Resoures.UnloadUnusedAssets()).
However I think it was experimental in 2019.1 and is only in preview state in 2019.3; It also requires .NET 4.x compatibility
https://docs.unity3d.com/Manual/UnderstandingAutomaticMemoryManagement.html

@Interkarma
Copy link
Owner

I'm waiting for 2019.4 LTS to be the "final" version that carries us to 1.0 and into the foreseeable future. If that incremental GC feature is out of preview in that version, I'd be happy to enable and see how it rolls.

@jefetienne
Copy link
Contributor

I can't seem to reproduce the issue, but I attempted a fix with #1782.

@petchema
Copy link
Collaborator Author

Just found this:
"Note that if you choose to use Resources.UnloadUnusedAssets, it is a very slow operation, and should only be called on a screen that won't show any hitches (such as a loading screen)."
https://docs.unity3d.com/Packages/com.unity.addressables@1.6/manual/MemoryManagement.html
But they don't provide any alternative way of unloading assets from AssetBundles...

@numidium
Copy link
Collaborator

UnloadUnusedAssets is called by UserInterfaceWindow.CloseWindow so that would explain why there are complaints of stuttering on window close.

@Interkarma
Copy link
Owner

UnloadUnusedAssets is called by UserInterfaceWindow.CloseWindow so that would explain why there are complaints of stuttering on window close.

Yep, scroll up and check Pango's first post :) It's not everyone though. I've never experienced this personally, even on my old 2012 clunker that most of DFU was built on prior to upgrading last year.

We do need to unload asset bundles and clean up intermittently though, as DFU never changes scenes (which is when most Unity games do this). DFU generates a TON of procedural assets that will sit around in memory unless purged every so often. The trick is finding a good spot to do so. On closing UI seemed reasonable at the time, but I'm not married to it. Happy to move this to indoor/outdoor transitions or after fast travel if that helps reduce the problem and still provide periodic cleanup.

@HankTheDrunk
Copy link

HankTheDrunk commented Apr 17, 2020

I did not experience this bug until 0.10.22 and it's made the game borderline unplayable with audio on. Closing any window causes the skip/stutter and it's maddening. I loot, sound stutter, I talk to an npc, sound stutter.
I know this post isn't helpful but I wanted to reply as a +1 I'm also having this problem. And for those that are it's damn near a deal breaker. And thank you to everyone who has worked on DFU it's truely remarkable!

@jefetienne
Copy link
Contributor

@HankTheDrunk thanks for letting us know.

Can you tell us your specs? Is it a full stutter that freezes the game or is it just the audio? Does it occur immediately when you start playing, or is it more like 15 minutes into gameplay? If it is immediate, does it also happen when going from the setup wizard to the video?

@rossipaolo
Copy link
Collaborator

Alternatively, I personally used a patch that leaves the call in UserInterfaceWindow.CloseWIndow() but throttles garbage collection so it doesn't happen more than once every 5 minutes, which is sufficient to prevent eventual long term memory usage runaways.

I think the original proposal by @petchema is a good idea for a simple and effective fix. Even if it might be only a partial solution, it should still be helpful given the number of reports that have been made recently and doesn't stop us from further discussion and improvements in the future.

@HankTheDrunk
Copy link

HankTheDrunk commented Apr 18, 2020

People who use a lot of mods are far more likely to experience this problem....

To test I spam clicked Escape on my keyboard. This activates the clearing call over and over. I run about 25 individual mods.
With many Mods the sound stutters for an extended period of time and is very noticeable
Without many Mods the sound doesn't stutter or only does so for less than 50ms

The reason I decided to try this simple test. I figured interkarma ran with minimal mods or none at all and hense he never has this stutter problem. So all the people annoyed about sound stutter are very likely running a lot of mods. However I am unsure if their system specs determine how many mods they can pile on before the stutter starts to happen noticeably.

@jefetienne I can't share my system spec's this account isn't secure, very sorry.

petchema pushed a commit to petchema/daggerfall-unity that referenced this issue Apr 18, 2020
Daggerfall Unity needs to explicitly call Resources.UnloadUnusedAssets()
because it barely uses SceneManager.LoadScene() which is the way games
usually indirectly call it.

This is currently done in two places, including while closing UI
windows, a path taken often enough during normal gameplay to prevent
unused assets to pile up too much.

However this can be a lengthy operation that blocks the whole
environment as it calls GC.Collect() internally, and can make sound skip
which can be pretty annoying once you noticed it (issue Interkarma#1539).

This patch makes sure the call to UnloadUnusedAssets() does not happen
more than once every 3 minutes, a tradeoff sufficient to make that issue
much less noticeable, yet should prevent unused assets to stay around
forever.

It should be noted that, for optimal efficiency, all code using
Resources.UnloadUnusedAssets() (say mods) should call either
ThrottledUnloadUnusedAssets() or ForcedUnloadUnusedAssets() instead to
prevent unnecessary collections.
@petchema
Copy link
Collaborator Author

I think the original proposal by @petchema is a good idea for a simple and effective fix. Even if it might be only a partial solution, it should still be helpful given the number of reports that have been made recently and doesn't stop us from further discussion and improvements in the future.

Sounds good enough for me, I submitted it as a PR

@petchema
Copy link
Collaborator Author

Another workaround that just occurred to me, if the only thing that can block in Resources.UnloadUnusedAssets() is indeed the call to System.GC.Collect(), then starting with Unity 2018.3 we could temporarily set GarbageCollector.GCMode to GarbageCollector.Mode.Disabled around the execution of UnloadUnusedAssets() so that GC.Collect() does nothing and UnloadUnusedAssets() is really as asynchronous as it claims to be.

@Interkarma
Copy link
Owner

I've wanted to move to 2019.4 LTS for a while now, it's just not available yet. What about moving to latest 2019.3 for now then it's only a short step to 2019.4 LTS once available?

I just want to minimise the number of engine shifts as it breaks mods. But that shift is coming and I see it as a vital step towards 1.0.

@petchema
Copy link
Collaborator Author

If binary compatibility is guaranteed between identical major versions, then that could be a plan.
Hopefully most mods will be upgraded, there's plenty of good stuff now but not all authors are still around and have enough time to maintain their mod(s) :(
I wonder if creating some kind of "preview" branch based on Unity 2019.x would help upgrading mods ahead or time, or if this will just increase the pain for everybody. I have no idea what are the best practices for such cases...

@Interkarma
Copy link
Owner

I'm only hoping the final jump from 2019.3 to 2019.4 LTS won't break mods again. I honestly can't guarantee that. My preference would be to wait for the LTS version, but I'm willing to try an incremental approach. The shift to linear lighting is another step that will break some mods. So fun times ahead leading into beta, I think.

We can create a 2019 preview branch. It could get a bit sticky though as the number of changes required is going to result in a lot of conflicts with new PRs. It's a safe way to explore problem space however, even if never merged directly back to master.

@Interkarma
Copy link
Owner

I've created a test branch for 2019.3.10f1 now. I don't plan to merge this one directly back to master, only to explore upgrade process further and release test builds to help modders prepare.

@petchema
Copy link
Collaborator Author

petchema commented May 7, 2020

This problem was mentionned in the forums again (about 0.10.22 for the record)
https://forums.dfworkshop.net/viewtopic.php?f=5&t=3687

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

No branches or pull requests

7 participants