#rb CarlMagnus.Nordin, Paul.Chipchase
#jira none
#preflight 62fdf44787319bacfb44bdcb
- deprecated and replaced the usage of StartAsyncLoading and CreateStreamingRequestForRange with a new batch API for requesting bulk data. The
new API fits better with the I/O dispatcher API and does not assume that the bulk data resides in the same I/O chunk. The new API also
removes state handling away from the bulk data instances.
[CL 21439218 by Per Larsson in ue5-main branch]
The double removal would occur in the following scenario...
1. LoadPackageAsync(PackageA)
2. GC & VerifyLoadMapWorldCleanup where PackageA is leaking
3. OnLeakedPackageRename(PackageA)
4. LoadPackageAsync(PackageA)
5. GC & VerifyLoadMapWorldCleanup where old PackageA is destroyed (and new PackageA is destroyed or leaking)
In development step 5 would cause the following assert in FLoadedPackageRef::FPublicExportMap::VerifyAllObjectsRemoved:
Error: Assertion failed: ObjectIndex < 0
In test/shipping step 5 would cause an FLoadedPackageRef assert or nullptr crash in
FGlobalImportStore::RemovePublicExports called from VerifyLoadMapWorldCleanup/OnLeakedPackageRename or CollectGarbageInternal/NotifyUnreachableObjects.
Tested BR replay and StW on PC.
#jira UE-159809, FORT-504960, FORT-494296, FORT-497087
#rb carlmagnus.nordin
[FYI] robert.millar
#rnx
#ROBOMERGE-OWNER: pj.kack
#ROBOMERGE-AUTHOR: pj.kack
#ROBOMERGE-COMMAND: _robomerge[bot4] UE5-MAIN
#ROBOMERGE-SOURCE: CL 21424567 via CL 21424762 via CL 21424834 via CL 21425163 via CL 21425777 via CL 21425803
#ROBOMERGE-BOT: UE5 (Release-Engine-Staging -> Main) (v977-21423386)
[CL 21426054 by pj kack in ue5-main branch]
#jira UE-159809
#rb none
#ROBOMERGE-AUTHOR: robert.millar
#ROBOMERGE-SOURCE: CL 21417438 via CL 21417445 via CL 21420908 via CL 21420929 via CL 21420961
#ROBOMERGE-BOT: UE5 (Release-Engine-Staging -> Main) (v975-21357124)
[CL 21421589 by robert millar in ue5-main branch]
Replace FinalizeInitialLoad with CompleteRegistrationEvent and use the same code path with/without the async loading thread.
Strip the FindAllScriptObjects logic from test and shipping builds.
Optimize initial load NotifyRegistrationEvent handling by passing the finished object instead of looking it up by name.
#jira FORT-492925
#rb carlmagnus.nordin
#rnx
#preflight 62fb3e5b4b04bcb92d500a68
[CL 21402146 by PJ Kack in ue5-main branch]
Increase log verbosity back to Display for RemoveUnreachableObjects.
Remove unnecessary FGCScopeGuard from the TickAsyncThreadFromGameThread.
#jira UE-160904
#rb carlmagnus.nordin
#rnx
#preflight 62f9fb9de67510be05328c1f
[CL 21384525 by PJ Kack in ue5-main branch]
Make import store more reliable by not relying on Object->HasAnyFlags(RF_WasLoaded | RF_Public) filtering (We can take the hit of checking all unreachable objects on the ALT)
The logic can be forced to run on the GT again with s.RemoveUnreachableObjectsOnGT=1, which in debug and development builds enables extra verifications.
Return actual number of destroyed objects from FAsyncPurge::GetObjectsDestroyedSinceLastMarkPhase to make these logs correct:
LogGarbage: 2.118 ms for incrementally purging unreachable objects (FinishDestroyed: 16176, Destroyed: 1571 / 16176)
LogGarbage: 2.303 ms for incrementally purging unreachable objects (FinishDestroyed: 16176, Destroyed: 1971 / 16176)
#jira UE-159941
#rb carlmagnus.nordin
#rnx
#preflight 62f4f8a7ad3bd8ad64107073
[CL 21353595 by PJ Kack in ue5-main branch]
#rb Per.Larsson
#jira UE-156189
#rnx
#preflight 62f4c098e60c9215b9bcde08
- The orignal name implied tha the payloads would only be stored locally on the users machine, but the intent is to describe a number of backends that are a) faster than the persistent storage backends b) no guarantee that the payload will be found.
- The new name better describes this functionality.
- We still accept 'LocalStorageHierarchy' but will log a warning informing the user to update their graph but this backwards compatibility code will most likely be removed before 5.1 ships.
-- EStorageType::Local will follow the usual deprecation rules.
- Not strictly related to this change but I also cleaned up and added additional loggng during the initialization of the virtualization manager.
[CL 21333601 by paul chipchase in ue5-main branch]
#rb Per.Larsson
#jira UE-156750
#rnx
#preflight 62f212e13b773d04161ee7dd
### Problem
- The payloads stored in map files tend to change more than other assets and would cause a lot more churn in the VA system.
- Some other systems like the landscape component are not able to sensibly continue if their payloads cannot be accessed (heightmaps for exmaple) and would prefer not to allow virtualization.
- As a short term fix we need an option to disable the virtualization on all payloads in map files. Future improvements to the filtering system will allow systems to more easily opt their payloads out of virtualization. When this is functional we might want to change the default from true to false.
### Feature
- The config optionf for this filtering is "[Core.VirtualizationModule]FilterMapContent=True"
- Testing if the owning UObject for a payload is in a umap can get tricky, because we not only need to check the umap but we also need to check if it is in a "_builddata.uasset" file, which is an additional file we store next to a umap containing things like lightmaps etc.
- At the moment we check for this by finding the outermost object for the given owner and check to see if it is a ULevel, UWorld or UMapBuildDataRegistry. This is a bit of a kludge but the types we need to check against are not accessible by this module and making them accessible will pull in a lot of dependencies that we'd prefer not to add.
-- One improvement might be to tag the FLinkerSave with the info we need and passing that into the serialization process rather than trying to work it out ourselves but I am wary of making that change until we are 100% sure that we want to keep this feature.
### Refactor
- Removed IVirtualizationSystem::IsDisabledForObject and replaced it with ::FilterPayload which can return multiple reasons for preventing a payload from virtualizing. (the method was added during 5.1 development so it should be fine to just replace it without deprecation)
-- The original behaviour for FVirtualizationManager::IsDisabledForObject has been moved to FVirtualizationManager::ShouldVirtualizeAsset
- Added a new header to declare enums/types used by the various parts of the virtualization system and started by moving EPayloadFilterReason there from the package trailer header. This allows both the core API and PackageTrailer to use EPayloadFilterReason without creating overburdened header dependencies.
-- EPayloadFilterReason has moved from the UE namespace to UE::Virtualization so the package trailer code needed updating accordingly.
- EditorBulkData will ask the virtualization system for the base filter reason, then add it's own reasons if UE_ENABLE_VIRTUALIZATION_TOGGLE is enabled. This bit of code will be removed for 5.1
[CL 21283179 by paul chipchase in ue5-main branch]
#rb Devin.Doucette
#jira UE-160218
#rnx
#preflight
- Before 21089705 we recorded the package path that were were saving an editor bulkdata to, but since we had no way to "attach" to the file on disk we would not load from it. The changes in 21089705 meant that we could try loading from the package, which fails in some odd ways when the editor domain is enabled.
- Adding the ::wasDetached flag when saving the package should prevent this and restore the previous behaviour.
[CL 21224848 by paul chipchase in ue5-main branch]
#rb Devin.Doucette
#jira UE-159985
#rnx
#preflight 62d6e8d2d76ea4b5032d5d56
### Problem
- When CL 21089705 was submitted, we changed the behaviour to assume that it is only safe to load payloads off disk if the AttachedAr pointer is valid rather than checking for the package path being valid. This allowed us to keep the package path intact after ::DetachFromDisk was called and give better error messages.
- However not all code paths were forcing the AttachedAr pointer to be set, TornOff bulkdata instances in the bulkdata registry for example causing these code paths to fail entirely.
- What we need is a way to not allow bulkdata instances that were attached but have been detached from loading (or at least force them to use the speculative loading path that was added in 21089705) and allow any other bulkdata instance that was never attached, to load it's payload as long as the path is set.
### Fix
- Instead of asking ::IsAttachedToPackageFile we instead ask ::HasAttachedArchive as in most places we are really checking if the AttachedAr pointer is null or not before we manipulate it
- There are two places where what we are really asking is if it is safe to load the payload from disk or not, these places now use a new method ::CanLoadDataFromDisk which checks that the bulkdata has access to a valid package path AND that the transient flag EFlags::WasDetached has not bee set.
-- This means that bulk data instances that are never attached to the packages FLinkerLoad can still load off disk, although this is less safe it reverts us back to the behaviour before CL 21089705was submitted.
### Misc
- Marked the transient flags as such in the code documentation.
- Ideally these flags would not have been added to the EFlags enum and a specific ETransientFlag enum added instead but the clean up for that would be irksome at this point.
[CL 21174617 by paul chipchase in ue5-main branch]
#rb none
#jira UE-159866
#rnx
#preflight 62d6930f164251d065d64113
- During 21147257 one of the checks to disable payload writing, that was calling ::IsReferencingByPackagePath was changed to checking the EFlags::ReferencesLegacyFile flag directly, which meant we were no longer taking referencing payloads from the workspace domain into account.
-- This was due to the CL being an attempt to merge 20965910 (which only checked the flag) with UE5 code (which called ::IsReferencingByPackagePath due to better editor domain support)
- This would cause the payload to be both referenced and written out into the editor domain, so on load we would end up with a flag telling the bulkdata to load from the original workspace domain file BUT the offset would reference where in the editor domain the payload was written out too.
-- This would only affect older versions of the package, newer versions would continue to use the package trailer to find the correct offet to use when referencing.
- This change requires that we change the EditorDomainVersion to clear out anything people might have saved with the incorrect info.
#ushell-cherrypick of 21169892 by paul.chipchase
[CL 21173890 by paul chipchase in ue5-main branch]
#rb Francis.Hurteau
#preflight 62d518cf3c3df32390a87183
- This is a rework of CL 20965910, to work with the newer code in editor bulkdata.
- bKeepFileDataByReference will be set to false as cooked packages will probably not be able to reference workspace domain payloads.
- When cooking we will use the legacy serialization path (i.e not to package trailer) BUT we will also not write out a payload at the moment.
#ROBOMERGE-AUTHOR: paul.chipchase
#ROBOMERGE-SOURCE: CL 21147257 via CL 21147267 via CL 21147273
#ROBOMERGE-BOT: UE5 (Release-Engine-Staging -> Main) (v972-20964824)
[CL 21153400 by paul chipchase in ue5-main branch]
#rb Per.Larsson, Matt.Peters
#jira UE-153904 , UE-156201
#preflight 62ce802c471a2c2886f592e8
### Problem
- Because of the package trailer we can easily find out which payloads are present in a package file and where they are. This means we don't need to rely on the cached offset into the file that bulkdata generally uses for payload loading. This means as long as the payload is still actually in a payload file then we can load it no matter what.
- We will still give errors in the following scenarios
-- The payload is not stored in a package trailer (either referencing older bulkdata or an older editor bulkdata format)
-- We cannot get a read handle to the package file.
-- The payload is no longer in the package file.
- If the attachment is still valid then we can guarantee that the payload can be loaded. Without an attachment we make our best efforts but in the case of the package file being reomoved/replaced this can fail. So ::ResetLoader will still be used to indicate that we want to remove the attachment but expect the package file to be changed and to load the payload into memory for safety. Work on improving this is another work item for the future.
### Fix
- We no longer remove the package path when ::DetachFromDisk is called nor do we remove flags relating to the payload. From this point onwards we use the attached archive being null to determine how we load. We continue to set OffsetInFile to an invalid value as we can no longer safely use it.
- Add a new method ::IsAttachedToPackageFile to explicitly show when we are checking to see if the bulkdata is still 'attached' to the package file.
- Explicitly set AttachedAr to null ptr when ::DetachBulkData is called on it. In theory the ::DetachBulkData call will call FEditorBulkData::DetachFromDisk which in turn will set AttachedAr but it can look confusing when reading the code. Explicitly setting the AttachedAr member to nullptr helps make the intent clearer.
- When loading the payload from disk there is now a new code branch that is followed if we detect that we are loading from a package file that we are no longer attached to.
-- This path gives bespoke errors in cases where it is not safe for us to try loading the payload, in addition there is a bespoke error message if loading from the package trailer fails.
-- Note we can only risk loading from the package trailer as it's format allows for us to find if the payload is still on disk or not. With the older formats we will not know if we are loading garbage or not.
-- Fixed a bug with CVarShouldLoadFromTrailer. When it is true we will only attempt to load payloads from the trailer if the payload is stored in the trailer. The comment for this development CVar has been updated.
- There has been a pass made to error reporting. If we are reporting an error relating to the package we will output the package name, if we are reporting on a error relating to the file itself then we will output the package path + extension
[CL 21089705 by paul chipchase in ue5-main branch]
#rb pj.kack
#ROBOMERGE-AUTHOR: robert.millar
#ROBOMERGE-SOURCE: CL 21060094 via CL 21060128 via CL 21060415 via CL 21060535
#ROBOMERGE-BOT: UE5 (Release-Engine-Staging -> Main) (v972-20964824)
[CL 21078246 by robert millar in ue5-main branch]
#rb PJ.Kack
#rnx
#preflight 62cc18391961b487b3757b5e
- MSVC Static Analysis picked up that the Class pointer was still being used in the branch where we know Class == nullptr.
- When iterating over the pending CDOs and logging them, we should be logging the class name for that CDO.
- Added an assert to check that CurrentClass is valid before using it.
- Fixed some code alignment issues that occurred in a recent code merge.
#ROBOMERGE-AUTHOR: paul.chipchase
#ROBOMERGE-SOURCE: CL 21053641 via CL 21053644 via CL 21053647
#ROBOMERGE-BOT: UE5 (Release-Engine-Staging -> Main) (v972-20964824)
[CL 21075787 by paul chipchase in ue5-main branch]
