Closed augustsaintfreytag closed 1 month ago
Hey, thank you for creating this PR! I will take a look at the code once I have some free time and start a review.
Regarding the 2.0.0 version: If your implementation works as it should, a user shouldn't even notice a difference to the previous version. So I guess a minor version bump is enough. I have plans on reworking the config system (which would be a breaking change). This would be worth of a 2.0 release (along with some other changes) IMO.
Thanks again! I will add the config options and do some tests.
Alright, that was quick! If you need something, you can always ping me here. I'll most likely check back from time to time anyway.
Alright, that was quick! If you need something, you can always ping me here. I'll most likely check back from time to time anyway.
Will do, thanks!
Just FYI, porting to 1.19.2, 1.20.4 and 1.20.6 went very smoothly. I'll probably release 1.4.0 very soon.
Thanks again for your great work!
EDIT: You can also add me on discord if you want (@henkelmax
)
Sweet! I guess I didn't have to touch a lot of the original code, it's mainly a very isolated addition on the side of the mod to call through so porting is pretty hassle-free.
This is my proposal to making Sound Physics Remastered thread-safe. Most of this branch implements what I already outlined in the original issue (https://github.com/henkelmax/sound-physics-remastered/issues/172#issuecomment-2080549494). Here's a brief summary of all the changes included:
Mainline
ClonedClientLevel
and associated models that store immutable level chunk data to allow safe sound computation (e.g. block type checking, clipping, raycasting).LevelAccessUtils
that can create and maintain a cached level clone on the main thread that is stored atomically and can safely be used from any other thread.tick
viatickLevelCache
.SoundPhysics
) only uses a new typeClientLevelProxy
to access level data.ClonedClientLevel
.USE_UNSAFE_LEVEL_ACCESS
, the proxy isUnsafeClientLevel
, a wrapper aroundClientLevel
owned by the main thread, mimicking the original behavior, for testing.Profiling & Logging
I also integrated a task profiler so I could gather some real world performance metrics and compare them to how the mod ran before. There is now a
TaskProfiler
that can gather aggregate measurements for a given kind of task, I've replaced the profiling inSoundPhysics
and added it to the clone operation. I also restructuredLoggers
and moved performance profiling in there (the conditional to log for performance used to be outside of that).The
TaskProfiler
works by creating an instance for a given context (e.g. sound eval, level clone), starting a profile with.profile()
, this returns a profile handle, and then later concluding the measurement with.finish()
on the profile handle. The profiler is also thread-safe so multiple measurements could run concurrently. It can be used to generate a tally of min/max/avg times, either by calling one of its accessors or callinglogResults()
(this usesLoggers.logProfiling
so it respectsperformance_logging
). Internally, it keeps the time measurements in a ring buffer so all the data returned is a rolling view of the last x number of profiles (configurable).Some preliminary metrics I collected (R = chunk range cloned, T = cache tick lifetime):
I think R4, T20 is a great optimised default, four chunks in all directions should be enough for any sound calculation (and they're infinitely high anyway so it would never miss a cave ceiling) and there is only so much that can change in the world in one second. Even if the player used WorldEdit or something exploded, I doubt anyone could ever notice a worst case single second latency in how the sound is filtered.
From these metrics, it seems not only is the cloning operation very fast but by reducing the bulk of level data made available to
SoundPhysics
, it also improves the performance of sound computation overall. And while cloning only happens every second or so, this is an improvement for all sounds occurring within that time window.This looks roughly like a ~32% performance improvement.
Sideline
Besides the main features, I also reformatted a tiny bit here and there, just couldn't help myself while I was looking around and getting familiar with the code, you can let me know if you don't want some of these minor changes. I was also using an extension in VS Code to sort and clean imports so any file I changed might also have had its imports cleaned, just as a heads up.
Questions
Some loose questions on some of the changes in no particular order:
var
, feel free to give your opinion on this. I usually work with languages where types are mostly omitted in assignments.fabric.mod.json
and Forge'smods.toml
), I hope this is alright with you.1.20.1
is the right destination branch for this merge request). I imagine someone would have to rebase a copy of this onto the other version branches and take care of any conflicts.That'd be it from me, I'm open to feedback.