DaggerfallBillboardBatch: mesh data creation process multi-threaded #2427
Conversation
|
Hey, thanks for the isolated update. Much easier to review. :) Just on a first pass test, I did get an exception (travelling to Daggerfall City).
|
Thank you for catching that! 121a594 should resolve it |
|
Hi again, I happen to be working on some stuff that involves using a custom atlas and billboard batches. Since I'm definitely feeling the performance hits there, I decided to give this new class a go and ran into a minor issue: The CreateMeshForCustomMaterial method errors out because it tries to read the cachedMaterial property. That property is only populated by the regular SetMaterial call that deals with Daggerfall's data, the SetMaterial call that handles the custom atlas does not attempt to do anything with it and there is no way to access cachedMaterial at the moment because of it's protection level. The mod I'm working on caches the atlases it creates so I was able to work around this by making the cachedMaterial property public and just setting it directly from my code (very bad Burt, I know). The SetMaterial call for custom atlases probably needs a CachedMaterial parameter so that mod authors can supply it properly. However this change fixed the error so I was able to test further. I can definitely see the performance increase, it feels so much smoother when moving between map pixels now even with two billboard batches per terrain. Would love to see this get adopted into the core. |
I think you should write a PR that proposes exactly what you need here. Especially since you can immediately field test it |
Apologies for the late reply, I've been rather busy but I agree, I'll do a PR for this. I came to realise that passing the cached material as the only parameter (instead of the material) is a cleaner way of doing it since the CachedMaterial already contains the material itself so it can be read from there. Since you've made the changes in this class standalone, I took the liberty of integrating it into my nature layout mod and the performance increase is mighty impressive. Instead of dealing with one billboard batch, I'm generating three batches at the same time, one for nature objects and the other two are used for billboard grass sprites. With the original billboard batches, this obviously caused a big hit in performance but with your class, it's reduced to a minor hickup when crossing map pixels. I haven't released or uploaded this version to my repository yet as it was just a test and your code is not mine to distribute but I would like to ask your permission to release it with your class included. |
Do it, it's yours now : )
|
Thank you, that's very kind. I'm currently working on the grass textures for the climates but once those are done I should be ready to release this version. I'll make sure to keep you posted.
I don't know about Burst but even without, the gain is substantial because of the multi-threading that occurs now and that's enough of a win for me :) |
|
I'll roll this one in for preview testing in next release. Well done @andrew-raphael-lukasik. :) |
|
Hey! I'm encountering a native array exception when loading some interior saves in current master after merging this PR. Stack trace and repro save below.
I'm getting close to next release. Let me know if you don't think this is an easy solve, and I'll revert PR changes for now. Cheers! :) |
|
Hit another exception after fast travelling.
|
|
The exception when loading interior saves can also cause towns not to load.
I'll revert this one for now, and we can approach again later. |
|
Goddamyt, it's hard to catch all of these. But fixes are very straigth-forward once there is a call-stack: 2e3f2f7 fixes 4723886 fixes
|
|
King Of Worms (@HybOj) noticed some regression with nature billboard shadows, https://forums.dfworkshop.net/viewtopic.php?t=6103 Git bisect pointed at commit 84e155a which is part of this PR |
|
Most likely mesh/object is no longer using double-sided shadow casting (i.e. backface of quad casts no shadows). This is something I fixed in my original implementation. Not sure if it's still possible using new implementation yet, I'll need to allocate time to look at this. Shadows on nature flats are essentially deprecated since 0.13 however. I left option in settings.ini, but it's not something I see a lot of need to support. If not possible to patch this, then I'll just remove option entirely. |
|
@petchema Actually, looks like you fixed this same problem back in 2017 in my original implementation. Same correction might work here. :) |
|
Yes, #1071 I must admit I have no idea what's going on and how this commit can affect shadows rendering; But that's a large commit full of Unity magic ;) |
|
Hmm, could be normal related then. I'll really need to unpack it again. Agree so much of new method is pure memory management voodoo. |
|
Guys, please dont scratch the Nature Shadows. I really hope this can be solved in a manner the functionality is kept in the game. Maybe Andrew can look into this as well, as he understands his code and its implications. Removing the nature shadows completely would be a big mistake. Thanks a lot, fingers crossed. |
|
I'm had a look at KoW's save on forums. At times you can see a spherical radius at the boundary where shadow casting is culled. Something about the mesh data must not be right, but interesting it only seems to affect shadow casting, not mesh visibility or lighting. |
Hi! This PR is part of #2416 saga.
What
I refactored #2416 changes to the
DaggerfallBillboardBatch.csto make them more conservative, safe and limited to a single file. It delivers less benefits than original, but will serve as a good foundation for future improvements which has been identified so far.I know you guys have other priorities now so no worries - take your time. I am posting this now while my mind is still familiar with this code.
Results
CreateMeshtakes less time from the main thread. Before: about266 ms(left), after: about34 [s](right):AddItemsAsyncwere added to open the door for future decrease in the overhead from the thousands of separateAddItemcalls (about an100 ms, sometimes more)ProfilerMarkerintroduced, to make profiling much more granular and informative. My controversial proposal here is in it's naming convention ie.____PascalCase(for methods) and____camelCase(for generic markers). I tried different schemes and this was my conclusion as most consistent and readable, but you might have different conclusions. Prefix___especially helps with readability as it makes these markers * immediately * apparent and distinguishable from the rest of the code mid-all these busy methods.