embeddedt commented 7 months ago

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

henkelmax commented 6 months ago

Please provide the full log files of this crash. Please also make sure this also happens in the latest version of the mod.

embeddedt commented 6 months ago

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.

CelestialAbyss commented 6 months ago

Here's a screenshot of one of the issues encountered on 1.19.2 after installing SPR:

henkelmax commented 6 months ago

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.

embeddedt commented 6 months ago

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.

IThundxr commented 5 months ago

This also causes https://mclo.gs/cWFrIAu by creating a CME when light updater is invoked since it is not thread safe

MehVahdJukaar commented 3 months ago

Why not just wrapping the level to avoid unsafe calls such as getBlockEntity? That seems to be what's causing the issue above

IThundxr commented 3 months ago

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.

NolanHewitt commented 3 months ago

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.

PaulRV0709 commented 2 months ago

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.

henkelmax commented 2 months ago

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

augustsaintfreytag commented 2 months ago

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.

PeglinX commented 2 months ago

just to be clear. Is this planned to be worked on?

Spiderach commented 2 months ago

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.

henkelmax commented 2 months ago

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!

augustsaintfreytag commented 2 months ago

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 ClientChunkCache to 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.

PaulRV0709 commented 2 months ago

LET'S GOOOO

uh i mean, hey that's pretty cool man

henkelmax commented 2 months ago

Awesome!

PeglinX commented 2 months ago

Kudos! “Sounds” great…

Delfite commented 1 month ago

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.

Poopooracoocoo commented 1 month ago

To anyone else about to comment to receive notifications on this issue, you can subscribe to issues! Just click the subscribe button!

augustsaintfreytag commented 1 month ago

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.

henkelmax commented 1 month ago

I'd love to merge it!

embeddedt commented 1 month ago

@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.

augustsaintfreytag commented 1 month ago

@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 commented 1 month ago

Do you think you will do this for the 1.19.2 version as well?

augustsaintfreytag commented 1 month ago

@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).

AtlasM8 commented 1 month ago

Ok. I decided to switch my mod pack to 1.20.1 anyway since writing that post. Keep up the good work!

ImBehinYa commented 1 month ago

"Hi, I just want to understand if it should work with create when I compile the fork. And when can we expect compatibility again?"

AtlasM8 commented 1 month ago

How is the progress looking. No pressure or anything just wondering.

augustsaintfreytag commented 1 month ago

@ImBehinYa Compatibility is defined within Create not here and they've flagged all versions of Sound Physics as incompatible. For testing, I just went into the fabric.mod.json in Create's JAR file and edited the version check for Sound Physics, you could give that a try if you want to demo my fork, mine is "2.0.0".

@AtlasM8 I got side-tracked a bit with other mod projects but I've put it all together now, I'm gonna open the merge request this weekend, just need to summarise my changes now, shouldn't be long!

henkelmax commented 1 month ago

Thank you @augustsaintfreytag for your PR!

I compiled both the Fabric and Forge version for people to test: sound-physics-remastered-1.20.1-1.4.0-pre1.zip (GitHub doesn't like Jars, so I had to zip them)

I'd love feedback from everyone that had concurrency issues.

IThundxr commented 1 month ago

@ImBehinYa Compatibility is defined within Create not here and they've flagged all versions of Sound Physics as incompatible. For testing, I just went into the fabric.mod.json in Create's JAR file and edited the version check for Sound Physics, you could give that a try if you want to demo my fork, mine is "2.0.0".

@AtlasM8 I got side-tracked a bit with other mod projects but I've put it all together now, I'm gonna open the merge request this weekend, just need to summarise my changes now, shouldn't be long!

use dependency overrides over modifying jars https://fabricmc.net/wiki/tutorial:dependency_overrides

RyanTheTechMan commented 1 month ago

Appears to be working on Quilt!

To bypass the Create dependency block, add this to your config/quilt-loader-overrides.json

{
    "schema_version": 1,
    "overrides": [
        {
            "id": "create",
            "breaks": [
                {
                    "replace": {
                        "id": "sound_physics_remastered",
                        "versions": "*"
                    },
                    "with": {
                        "id": "sound_physics_remastered",
                        "versions": "<1.4.0"
                    }
                }
            ]
        }
    ]
}

p.s. I have been putting together a modpack over the last few days and I was JUST about to say bye to Sound Physics, but I'm glad I saw this! Thanks for all the hard work!

augustsaintfreytag commented 1 month ago

Only just realised that you're one of the maintainers of Create, @IThundxr, nice to see you here, too. I didn't know about the fabric_loader_dependencies.json, I just hacked it in to test it, this should be the way for anyone wanting to test the preview build, though, I agree.

@RyanTheTechMan, you could replace the * with >=1.4.0, that's where we'll be at with this patch, only the preview build could satisfy this at the moment so no one can accidentally load an outdated version.

If @CelestialAbyss still follows this thread, I'd be very interested if they still see these artifacts they showed above, just to have another confirmation. Technically there should be no way they still appear.

RyanTheTechMan commented 1 month ago

@RyanTheTechMan, you could replace the * with >=1.4.0, that's where we'll be at with this patch, only the preview build could satisfy this at the moment so no one can accidentally load an outdated version.

Just edited the message. My b

Edited to <1.4.0 as it breaks anything below 1.4.0, but not 1.4.0 itself.

henkelmax commented 1 month ago

Version 1.4.0 has now been released as alpha for Minecraft 1.19.2, 1.20.1, 1.20.4 and 1.20.6. I'll keep this issue open for a while. Please let me know if there are any issues.

Thanks again @augustsaintfreytag for fixing the issue and even improving the mods performance!

embeddedt commented 1 month ago

I can confirm that 1.4.0 fixes the symptoms reported in my initial post - there are no longer CMEs and other errors printed in the log when breaking belts, and belts no longer continue rendering after being broken.

henkelmax commented 1 month ago

I just released 1.4.1. This version fixes some potential issues with changing levels or leaving/joining the game.

Poopooracoocoo commented 1 month ago

Would have left this comment on a different issue but I was 10 minutes late and it got locked lol.

The next version of Create Fabric is expected to remove that declaration of incompatibility. https://github.com/Fabricators-of-Create/Create/commit/b12d4d839d3b5a3bb707e4a5a75c9055f8b5f274

Until then, you can create a Fabric dependency override:

Create a file in your config folder with the name fabric_loader_dependencies.json and the following contents:

{
  "version": 1,
  "overrides": {
    "create": {
      "breaks": {
        "sound_physics_remastered": "<1.20.1-1.4.0"
      }
    }
  }
}

henkelmax commented 1 month ago

Would have left this comment on a different issue but I was 10 minutes late and it got locked lol.

The next version of Create Fabric is expected to remove that declaration of incompatibility. Fabricators-of-Create/Create@b12d4d8

Until then, you can create a Fabric dependency override:

Create a file in your config folder with the name fabric_loader_dependencies.json and the following contents:
{
  "version": 1,
  "overrides": {
    "create": {
      "breaks": {
        "sound_physics_remastered": "<1.4.0"
      }
    }
  }
}

Your override is not correct. <1.4.0 would also allow 1.20.1-1.3.0, as your check only checks the first part of the mod version which is the Minecraft version.

KostromDan commented 1 month ago

One user of my modpack reported deadlock. Idk deadlock related or not with SPR but can see 10k lines of spam from SPR mod before deadlock.

[02:30:11] [Sound engine/WARN]: Can not return client level proxy, client level clone has not been cached. This might only occur once on load.

2024-05-26-3.log

henkelmax commented 1 month ago

This should already be fixed with 1.4.2

henkelmax commented 1 month ago

In case it still happens, I'd appreciate if you'd send me logs of that version again.

Fyoncle commented 3 weeks ago

What about Create compatibility? Will it be fixed or no

IThundxr commented 3 weeks ago

What about Create compatibility? Will it be fixed or no

that's fixed in dev already

Fyoncle commented 3 weeks ago

What about Create compatibility? Will it be fixed or no

that's fixed in dev already

Oh yea just noticed, sorry. Seems like we need to create a config to fix it, thats probably now in Create's side since they marked the mod as broken.

henkelmax / sound-physics-remastered

Mod unsafely accesses client world off-thread #172

🟡 Disadvantages

🟢 Advantages

LET'S GOOOO