[Patch] Say why a rendering extension's shader library or pipeline failed to load - #1215
Conversation
|
Wow, 170 commits. can we squash them please. |
|
Also, it seems that some files that were changed have nothing to do with the PR description. For example, MotionDatabase.swift. I think you just wanted to do a PR on bcff749 |
|
let me review because theres too much and wasn't suppose to be there. |
…iled to load When a plugin's metallib could not be created, RenderShaderLibraryManager discarded the error Metal threw and recorded a bare `metallibCreationFailed`, so the only trace of a plugin that rendered nothing was a warning naming the resource. The case that motivated this was a metallib compiled by Xcode 27 without -mtargetos and rejected on a visionOS 26 device with "This library is using a deployment target that is not supported"; that text never reached the log. - `RenderShaderLibraryLoadingError.defaultLibraryCreationFailed`, `.metallibCreationFailed` and `.libraryCreationFailed` carry a `reason` (the NSError's localizedDescription for Metal and Foundation errors, the Swift description otherwise) and print it. - `RenderExtensionPipelineError.creationFailed` carries a `reason` too. The pipeline creator protocol now throws, and `CreatePipeline` and `CreateComputePipeline` are wrappers over throwing cores (`buildRenderPipeline`, `buildComputePipeline`) so the Metal error reaches the registry; their own `handleError` message now includes it as well. The legacy init-block registrations say whether the block returned nil or a pipeline with `success == false`. - Extension shader-library and pipeline failures are logged with `Logger.logError` instead of `logWarning`, matching the engine's own metallib failure path. Source compatibility: constructing or exhaustively matching the four cases above needs the new `reason` argument; nothing else changes. Only the engine and its tests construct them.
|
Sorry about that. The branch was cut from our fork's develop, which carries motion-matching work that isn't upstream yet. Rebased onto upstream develop: one commit, 8 files, only the shader-diagnostics change. |
bcff749 to
af6864f
Compare
|
Hey @miogds , nice fix overall — this is going to save a lot of head-scratching next time a plugin silently fails to render. One thing I noticed while going through it though: In } catch {
handleError(.pipelineStateCreationFailed, "\(name): \(failureReason(for: error))")
}By the time execution reaches this branch, when what we actually want (and what the extension path already gets right!) is just: It seems you already wrote the fix for this — Easy fix, something like: } catch {
let reason = (error as? PipelineCreationError)?.reason ?? failureReason(for: error)
handleError(.pipelineStateCreationFailed, "\(name): \(reason)")
return nil // just for CreatePipeline
}in both spots — or just call Also, totally optional, but a quick test asserting the final log string here (or at least Thanks |
… line Review follow-up on untoldengine#1215. CreatePipeline and CreateComputePipeline passed the thrown PipelineCreationError to failureReason(for:), which did not know the type and fell back to String(describing:), so the engine's own log line read `pipelineStateCreationFailed(underlying: Error Domain=...)` instead of Metal's message. The extension path was unaffected because it unwrapped the error itself. PipelineCreationError is now a LocalizedError whose errorDescription is its reason, so failureReason(for:) is the one place that describes a thrown error for both paths, and the registry's separate describe helper is gone. A unit test asserts failureReason(for:) against a PipelineCreationError wrapping a Metal NSError.
|
Good catch, thanks. |
Say why a rendering extension's shader library or pipeline failed to load
When a plugin's metallib could not be created,
RenderShaderLibraryManager.loadcaught the error Metal threw and recorded a baremetallibCreationFailed(libraryID:resource:subdirectory:). The registry then logged one warning,Extension '…' cannot load shaders: Failed to create shader library '…' from bundled metallib '….metallib', removed the extension and invalidated the plugin. A plugin that rendered nothing left no explanation behind.The case that motivated this (untoldengine/UntoldArcade#23): a plugin metallib compiled by Xcode 27 without
-mtargetoswas stamped for visionOS 27, and Metal rejected it on a visionOS 26 device withThat text never reached the log, so the reviewer only saw "not working". The engine's own metallib path (
createXR→handleError(.metalLibraryNotFound, error.localizedDescription)) already logs the reason at error level; the extension path now does the same.Changes
RenderShaderLibraryLoadingError.defaultLibraryCreationFailed,.metallibCreationFailedand.libraryCreationFailedcarry areason: Stringand end their description with it. The reason is theNSError'slocalizedDescriptionfor Metal and Foundation errors,errorDescriptionfor aLocalizedError, andString(describing:)otherwise, so a plain Swift error reads as its case name instead of "The operation couldn't be completed".RenderExtensionPipelineError.creationFailedcarries areasontoo. The internal pipeline creator protocol now throws, andCreatePipeline/CreateComputePipelineare thin wrappers over throwing cores (buildRenderPipeline,buildComputePipeline, internalPipelineCreationError), so Metal's error reaches the registry. Their public signatures and behaviour are unchanged, except that their ownpipelineStateCreationFailedlog line now includes the reason as well. The legacy init-block registrations say whether the block returnednilor a pipeline withsuccess == false.Logger.logErrorinstead oflogWarning. Resource-validation, conflict and graph-validation rejections still log as warnings; I left those as they were.With this change the log line for the arcade case reads:
Source compatibility
Constructing or exhaustively matching the four cases above needs the new
reasonargument; nothing else in the public API changes. Only the engine and its tests construct them (the editor, arcade and examples do not reference these cases). I did not givereasona default value on purpose: a silently empty reason is the gap this fixes.Tests
Tests/UntoldEngineTests/RenderExtensionDiagnosticsTests.swiftcovers every case's description and the reason helper, including a Metal-styleNSErrorcarrying the deployment-target message.RenderShaderLibraryPackagingTest: the fake loader throws a configurable error; the invalid-metallib test expects the reason, and new tests cover a MetalNSErrorreason, a failing default library and a failing URL library.RenderExtensionPipelineDescriptorTest: the fake creator throws; the two creation-failure tests expect the reason, and new tests cover compute creation failure and an init block returning an unsuccessful pipeline.Verified locally on Xcode 27.0:
swift test --filter UntoldEngineTests, the two render-extension test classes,swift buildof every product, the externalSwiftPackagePluginfixture, andswiftformat --lint --swiftversion 5.8on the changed files.