sinbad / SPUD

Steve's Persistent Unreal Data library
MIT License
300 stars 44 forks source link

World Partition support #45

Closed cbQB closed 1 year ago

sinbad commented 2 years ago

Oh hey, thanks for this! I don't have UE5 installed yet, guess I should get on that soon :)

@Daboiowich: you were looking for World Partition support (#42), perhaps you can run this through its paces & comment?

Daboiowich commented 2 years ago

Hi! It seems it's working but I have started getting this crash: image

EDIT: I'll update nvidia drivers and verify UE files and I'll let you know if it happens again. If it happens I'll also disable SPUD to check that it's only happening with SPUD on the project, thanks!

EDIT 2: Still happening, after installing debug symbols for the editor, the crash gives more information: image

EDIT 3: This crash doesn't seem to happen in Standalone Game, neither in build I suppose

cbQB commented 2 years ago

@Daboiowich it would help if you could attach VS when that crash occurs and let me know the value of NamePrivate (the variable whos value GetFName() returns). Or if you could send a minimal project that reproduces the problem.

cyphusx commented 1 year ago

I've been looking at this crash in SubscribeLevelObjectEvents, and have spotted two things.

First, the code in this change registers USpudStreamingLevelWrapper::OnLevelShown as a listener for OnLevelShown in USpudSubsystem::Tick for every new streaming level. This, when fired, calls USpudSubsystem::HandleLevelLoaded, which in turn calls SubscribeLevelObjectEvents. Of course, OnLevelShown is fired for all of the streamed levels at startup.

If the player is also within an ASpudStreamingVolume at startup, the ultimate call to USpudSubsystem::LoadStreamLevel will cause HandleLevelLoaded to be called once the load has completed.

This is the source of the double register, which causes the ensure on startup in SPUDExamples.

Secondly, even if you don't have an ASpudStreamingVolume, it is possible to cause SubscribeLevelObjectEvents to be called twice when using world partitioning by causing a cell to quickly cycle between shown and not shown (e.g. by quickly walking backwards and forwards). This is due to the fact that world partitioning may reuse a loaded but recently hidden cell when given a request to show that cell again. This can cause actors to have their BeginPlay called twice, and crucially, it means those actors already have bindings for OnActorDestroyed.

The simple solution to both of these issues is to update SubscribeLevelObjectEvents to use an AddUniqueDynamic, rather than AddDynamic when adding a listener for OnDestroyed - however this is likely to be slower than the AddDynamic, and so for large worlds with lots of actors may cause a performance hit, however especially in the case of the reuse of a cell, I'm not sure if there is a better alternative.

sinbad commented 1 year ago

Thanks for taking a look. We're now using UE5, I've been busy with other work (like making SUDS) but I have a little time next week so plan to take another look at this PR. I don't actually have UE4 installed anymore so will probably just branch a legacy UE4 version here either way and look to integrate this as into a UE5 only version for future use.

sinbad commented 1 year ago

I've made a few changes, including eliminating the double-save/load when using the SPUD streaming volumes, since everything works fine based on show/hide for even regular streaming volumes now. I've left the flag in which flips between the old / new methods in case people need legacy support, but I think most will be fine using the WP path now (and I've renamed that boolean appropriately).

I did change to AddUniqueDynamic; although it's technically more costly, it doesn't seem to be a big deal. I don't think it's actually needed now I've removed the higher level duplication, but I figure why not be safe anyway.

This is now merged in and WP is officially supported. I still haven't used it much myself but I built a test WP level in the SPUDExamples project and everything seems to work fine.