sound-physics-remastered
sound-physics-remastered copied to clipboard
henkelmax
Mod unsafely accesses client world off-thread
Bug description
Sound Physics Remastered accesses the client world from the sound thread. The client world is not really thread-safe and doing this can lead to all sorts of issues. In my case, it seems to cause ghost Create mechanical belts to occasionally render in the world with Embeddium. I am not 100% sure of the exact action that causes that issue, but it seems related to block retrieval off-thread triggering creation of block entities as well as other side effects. A stack trace from my debug mixin is attached below so you can see what I mean (it's using SRG names, sorry about that).
To fix this issue properly I believe the mod will need to be refactored so that all block retrieval is done on the main thread and stored in a snapshot data structure that the sound thread then accesses. Otherwise, this will likely continue to cause all sorts of subtle problems that will be very difficult for players/other mod authors to track down.
This issue was discovered on 1.20.1 but likely affects earlier versions too unless the code changed significantly.
java.lang.Exception: null
at net.minecraft.world.level.chunk.LevelChunk.handler$zbb000$yellForOffThread(LevelChunk.java:1338) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.chunk.LevelChunk.m_62934_(LevelChunk.java) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.chunk.LevelChunk.m_5685_(LevelChunk.java:315) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.Level.m_7702_(Level.java:560) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at com.simibubi.create.foundation.block.IBE.getBlockEntity(IBE.java:72) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at com.simibubi.create.foundation.block.IBE.getBlockEntityOptional(IBE.java:53) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at com.simibubi.create.content.kinetics.belt.BeltBlock.m_5939_(BeltBlock.java:370) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at net.minecraft.world.level.block.state.BlockBehaviour$BlockStateBase.m_60742_(BlockBehaviour.java:684) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B,pl:mixin:APP:modernfix-common.mixins.json:bugfix.chunk_deadlock.BlockStateBaseMixin,pl:mixin:APP:ferritecore.blockstatecache.mixin.json:BlockStateBaseMixin,pl:mixin:APP:modernfix-common.mixins.json:perf.reduce_blockstate_cache_rebuilds.BlockStateBaseMixin,pl:mixin:A}
at net.minecraft.world.level.ClipContext$Block.m_7544_(ClipContext.java:62) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at net.minecraft.world.level.ClipContext.m_45694_(ClipContext.java:40) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at net.minecraft.world.level.BlockGetter.m_151358_(BlockGetter.java:63) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.world.level.BlockGetter.m_151361_(BlockGetter.java:119) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.world.level.BlockGetter.m_45547_(BlockGetter.java:58) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.raycast(SoundPhysics.java:618) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.evaluateEnvironment(SoundPhysics.java:287) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.processSound(SoundPhysics.java:180) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.onPlaySound(SoundPhysics.java:147) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.mojang.blaze3d.audio.Channel.handler$zjj000$play(Channel.java:533) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SourceMixin,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:ChannelAccessor,pl:mixin:A}
at com.mojang.blaze3d.audio.Channel.m_83672_(Channel.java) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SourceMixin,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:ChannelAccessor,pl:mixin:A}
at net.minecraft.client.sounds.SoundEngine.lambda$play$6(SoundEngine.java:404) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,pl:runtimedistcleaner:A,re:classloading,pl:accesstransformer:B,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SoundSystemMixin,pl:mixin:A,pl:runtimedistcleaner:A}
at net.minecraft.client.sounds.ChannelAccess$ChannelHandle.m_120157_(ChannelAccess.java:34) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.util.thread.BlockableEventLoop.m_6367_(BlockableEventLoop.java:156) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.util.thread.BlockableEventLoop.m_7245_(BlockableEventLoop.java:130) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.util.thread.BlockableEventLoop.m_18701_(BlockableEventLoop.java:139) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.client.sounds.SoundEngineExecutor.m_120336_(SoundEngineExecutor.java:42) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at java.lang.Thread.run(Thread.java:840) ~[?:?] {re:mixin}
Mods installed
- NeoForge 47.1.79
- Embeddium 0.2.9
- Create 0.5.1f
- FerriteCore 6.0.1
- ModernFix 5.9.3
- Sound Physics Remastered 1.20.1-1.2.1
Please provide the full log files of this crash. Please also make sure this also happens in the latest version of the mod.
This isn't a specific crash report. It's a technical explanation of a design flaw that I believe is responsible for causing random glitches or crashes in other mods, plus an example stacktrace outlining how the problem occurs.
Here's a screenshot of one of the issues encountered on 1.19.2 after installing SPR:
The mod did always access the client world from another thread. This was already a thing back then for the original mod from Sonic Ether. The mod is designed around being able to do this. I also think this mod wouldn't be feasible if blocks need to be accessed from the main thread. If all evaluation rays need to be casted in the main thread, the game frame rate would be unplayable. For now this has always worked without any issues. Its definitely possible that this causes the issues with the create mod, but usually issues from non-thread safe operations are way more inconsistent, so I'm assuming create does some very weird hacky stuff to the world rendering or changes very fundamental stuff. I also always used the mod in conjunction with Sodium and never had any issues. Maybe Embeddium does some things differently. Interactions from off thread are also limited to read-only calls, which in general shouldn't cause many issues.
All I can do is declare Create or Embeddium as an incompatible mod, as its impossible to make the mod "Thread safe". If anyone has any ideas on how to work around this issue, please let me know, but Thread safety isn't really possible to my knowledge.
If anyone has any ideas on how to work around this issue, please let me know, but Thread safety isn't really possible to my knowledge.
The only way to do it correctly (as far as I know) is to make a snapshot of the needed block data on the main thread, and then work with that on the sound thread (without ever accessing the client world directly). This is the same approach vanilla uses to update the chunk meshes using worker threads (16x16x16 snapshots are made on main and then used by the workers).
Anyways, the next release of Embeddium will warn users that support won't be provided if Sound Physics (or other unsafe threading mods like Entity Culling) are installed, as I'm getting too many concurrency-related bug reports which are not caused by me, and that I don't have an easy way to fix.
This also causes https://mclo.gs/cWFrIAu by creating a CME when light updater is invoked since it is not thread safe
Why not just wrapping the level to avoid unsafe calls such as getBlockEntity? That seems to be what's causing the issue above
Marked SPR as broken in create fabric due to the mass amounts of concurrency issues we've gotten, If the suggestion above works and fixes this please let me know so i can remove the breaks.
The suggestion above looks like it would fix it to me. But only way to know for sure is for it to be implemented and tested.
The create latest patch actually added an incompatibility flag for this mod, so now you can't play with both installed because of this non thread safety.
The create latest patch actually added an incompatibility flag for this mod, so now you can't play with both installed because of this non thread safety.
Yes, as mentioned here:
https://github.com/henkelmax/sound-physics-remastered/issues/172#issuecomment-2016848783
Sound Physics is an amazing mod and is definitely worth it to see this change implemented. I'd say it gives it some solid future-proofing to use an access-by-snapshot approach, especially if vanilla already does it this way.
just to be clear. Is this planned to be worked on?
Looks like it was reopened, pinned, and marked as a bug. If not immediately it looks like there will at least be an attempt to fix it.
Looks like it was reopened, pinned, and marked as a bug. If not immediately it looks like there will at least be an attempt to fix it.
Yes, I currently don't have the capacity to look into it, but its on my list. If anyone is interested to take a look at it or make a PR, any help is always appreciated!
So I decided I didn't want to live without Sound Physics (mainly because of the Voice Chat integration) so I took a look at a possible implementation myself. I took the direction @embeddedt proposed here and wrote a working thread-safe proof of concept.
I checked what data SPR actually uses and it's pretty straightforward, block state and clip for everything from materials to raycasting (reflection and occlusion). The team behind Sodium/Embeddium also creates snapshots in a LevelSlice model, though obviously, you need much more of the level data for rendering than for sound simulation so I was able get by with cloning less than they do.
My approach lets SPR work entirely off a sparse level snapshot inside a ClientLevelProxy that offers the functionality otherwise provided by ClientLevel. Currently, I create it for every run of evaluateEnvironment, though that can be optimised. It takes the player's position and copies block data in a configurable radius into a collection of ClonedLevelChunk models. This copy is completely decoupled from ClientLevel but provides everything we need, block states, fluid states, height maps. SoundPhysics itself then creates a proxy once and uses that instead of the full level with the exact same signatures as before.
As I see it, this is the breakdown for what I created:
🟡 Disadvantages
- Introduce a brief halt having to synchronize the client level's
ClientChunkCacheto clone it. During initial testing, the performance impact for this is neglible, even with a naive and overeager approach (copy for every sound). There is immense potential for optimisation, e.g. by keeping level data around for more than one sound event, reducing caching radius, etc. - Slightly increase memory use and bandwidth to accommodate the cloned chunk data, though this can also be optimised by reusing copied data or reducing the copied chunk radius.
🟢 Advantages
- Make sound simulation thread-safe in regards to level data and whatever happens on the main thread while keeping everything on the sound thread.
- Fix threading-related errors where accessed level data is invalid, solve incompatibility with Create and other mods yet unreported.
- (New) Potentially increase overall sound simulation performance by allowing the mod to only use part of the available level data for evaluation (i.e. use an 8x8 chunk radius around the player instead of the whole client-loaded level for operations like raycasting). The radius to be cached can be configured.
I'm going to refine and clean up my branch a bit but I wanted to announce this here now that I can confirm it's working. I also tested it in a big pack of 400+ mods (including Create and addons) and haven't encountered any issues there.
So I decided I didn't want to live without Sound Physics (mainly because of the Voice Chat integration) so I took a look at a possible implementation myself. I took the direction @embeddedt proposed here and wrote a working thread-safe proof of concept.
I'm following this thread. Post a link to the mod in here once you upload it. I will definitely be trying out your version when it drops.
To anyone else about to comment to receive notifications on this issue, you can subscribe to issues! Just click the subscribe button!
I'm going to open a merge request soon if @henkelmax would want this to be merged into their fork after review, there are just a few small things I want to add and I haven't had a chance to profile it on the actual numbers, then I'll get to it.
I think it'd be in the community's best interest if I don't create an entirely separate fork of Sound Physics with these changes and instead, we use what I did to reinstate this project and Max's work on the mod.
If anyone is overeager to test it, I do have my fork as a public repo, all my WIP changes are on feature/thread-safety, you can compile it and test it on 1.20.1. All the debug tools already included in Sound Physics work just as fine.
@augustsaintfreytag My only feedback on your current implementation is that you currently create the level snapshot in SoundPhysics.evaluateEnvironment, which is already running on the sound thread. For proper thread safety the snapshot needs to be created on the client thread instead.
Since from what I can tell, you just copy a range of chunks around the player, the simplest solution might be to maintain the snapshot in a client tick event, and update it every 20 ticks, or when the player has moved a given distance. The sound thread can then just use the snapshot being maintained by the client thread.
@embeddedt I thought about that approach, too, to always have something like an atomic level clone ready on the main thread that'd be safe to access from the sound thread. My current draft synchronizes access to the ClientChunkCache during the clone in ClonedClientLevel so the data is locked during the read operation. I can get on board with moving from this "cache-on-sound" to a "cache-always" approach, sounds probably happen frequently enough to warrant it.
All in all, I think this discussion might be best held in the merge request later. Like I said, this was my proof of concept implementation, I'm happy to refine it with you all. I also have a few mod-specific things to ask Max on what we might want to make configurable, e.g. caching/eval radius or cache retain time.
I appreciate you checking it out already, your Discord was the one I was thinking to ask around before I started on the implementation.
@AtlasM8 I don't see a reason it couldn't be ported to all versions Sound Physics currently supports (which seem to be 1.19.2, 1.20.1, and 1.20.4). From my changes, it only depends on how much the internals of levels have changed between releases. I haven't built on the most recent Minecraft version myself, I branched off 1.20.1 because it's the mod LTS right now (and it's the one I play on).
Ok. I decided to switch my mod pack to 1.20.1 anyway since writing that post. Keep up the good work!
"Hi, I just want to understand if it should work with create when I compile the fork. And when can we expect compatibility again?"
