Some improvements to the SharedAsset system #971
Conversation
| /** | ||
| * @brief Determines if this list contains a given node in constant time. In | ||
| * order to avoid a full list scan, this method assumes that if the node has | ||
| * any next or previous node, then it is contained, then it is contained in |
There was a problem hiding this comment.
Small typo here. "then it is contained, then it is contained"
| * be at least one error in this list. | ||
| */ | ||
| ErrorList errors; | ||
| }; |
There was a problem hiding this comment.
If we indeed end up using this type for more than just this feature, it would be lovely to add a lot of the convenience features you find with Rust's Result type. For example, a match method that runs one lambda if Ok, or another lambda if Error. Or a map_value method than can turn a Result<T, E> into a Result<T2, E> using a provided lambda. We don't need to add these now, just thinking out loud.
There was a problem hiding this comment.
Would it be possible to build on std::expected? I think we're already using expected-lite here in native.
There was a problem hiding this comment.
I actually used nonstd::expected in an early iteration of this, but the problem is it doesn't offer a good way to represent "success, with non-fatal warnings." Which is a fairly common phenomenon in cesium-native.
I definitely agree that we would benefit from a little work on ErrorList to improve its ergonomics, though. Starting with a rename, because it's weird to call it ErrorList when it also holds warnings.
This is a PR into #926.
SharedAssetDepotis now templatized on both the asset type (such asImageAsset) and on the type of the unique key used to identify specific assets. Previously the unique key was always a string (the URI).IDepotOwningAsset, which is the interface by whichSharedAssetreferences theSharedAssetDepotthat owns it. And moved most of the depot-specific fields out ofSharedAssetand into privateAssetEntryclass in theSharedAssetDepot. This approach allows us to avoid templatizingSharedAsseton the asset key type.NetworkAssetDescriptorandNetworkImageAssetDescriptor, which are classes intended to be unique keys for identifying assets.TilesetSharedAssetSystem. This is just a bit of future-proofing to allow us to have tileset-specific shared assets in the future.pAssetDepotinTileContentLoadInfotopSharedAssetSystem, because it's not an asset depot.#includeofAsyncSystem.hfromIAssetAccessor.h. OnlyFutureis needed, notAsyncSystem.SharedAssetcopy constructor and assignment operators protected. It's ok to copy anImageAsset, but copying aSharedAsset<ImageAsset>would result in slicing.storemethod fromSharedAssetDepot, because dealing with the edge cases of this (such as calling it after a request to get the asset is already in flight) was tricky, and it's probably not very useful anyway. This means that only assets created by the depot can be associated with it, which is probably fine.std::functionrather than an interface, since it's just one function.SharedAssetDepot:staleAssetSizeLimit->inactiveAssetSizeLimitBytesfor consistency with terminology used elsewhere.getDistinctCount->getAssetCountgetDeletionCandidateCount->getInactiveAssetCountgetDeletionCandidateTotalSizeBytes->getInactiveAssetTotalSizeBytesSharedAssetDepot::getActiveCount.SharedAssetDepot::getUsageCount, because it's peering into implementation details, and I don't think it's needed.markDeletionCandidate/unmarkDeletionCandidatetake a const asset, since conceptually they modify the depot not the asset.CesiumUtility::Hash::combinefunction, ported from Boost. Updated a couple of places we were combining hashes to use it.GltfSharedAssetSystem::getDefaultno longer takes an "options" object. Which is good because it was always ignored after the first time the method was called.isOrphanfromDoublyLinkedListPointers, because it seems weird to call a node an orphan when its the only item in a list, and we can't distinguish that case from an actual orphaned node. Added acontainsmethod toDoublyLinkedListinstead. It works in constant time with the caveat that it can'd distinguish "in this list" from "in some other list". The caveat is documented.ErrorListto create an instance with one error (error) or one warning (warning).CesiumUtility::Result<T>, which is is a handy container for some result instance plus an ErrorList to capture any warnings and errors that occurred while producing it (or failing to produce it). Also addedCesiumUtility::ResultPointer<T>for convenience when the value happens to be an IntrusivePointer. There are a few places in the code that have custom containers for the same purpose, such asGltfReaderResult. We should switch them to use this type instead, but it need not be part of this effort.