Conversation
|
You may want to refresh yourself on Bevy's AI Policy, it was updated recently. |
|
Thanks for the feedback! I apologize sincerely for the initial messy state of the comments in this PR. Full disclosure: I had used Claude for some initial research into this, to review my work, and to help write some documenting comments. This area of the Bevy code base is new to me, so I had leaned into it to verify my approach and my thinking. I should have done more due diligence into my own work and into the updated AI policy before opening this PR for review, and I should have disclosed the AI usage in my PR description. I won't do this in the future. In the interest of full transparency, I updated my PR description with an AI disclosure. |
alice-i-cecile
left a comment
There was a problem hiding this comment.
Can you please add an AI use disclosure to your PR description so we can review it more accurately?
Done! I put it at the top of the description. |
beicause
left a comment
There was a problem hiding this comment.
Thanks for digging into this — the diagnosis looks right to me. import a::b::C can't be disambiguated from the import statement alone, and pre-scanning both readings just to probe one of them is what produced the 404.
My position: this PR introduces more tradeoffs than the bug fix justifies. I don't think this is the right direction:
- Asset loading moves into the render world, and the dependency's lifetime and load state leave the asset system along with it.
PipelineCache::load_missing_wesl_modules_systemnow callsasset_server.load()and takes on the dependency's lifetime, which used to be the loader's job. Your review note flags this as the part you're least sure about, and I think that instinct is worth trusting — it's a tradeoff that comes with two consequences. First,wesl_module_requestsis never pruned, and the follow-up you mentioned — dropping entries onAssetEvent::Removed— looks fragile to me when it comes to unloading dependencies. Second, because these imports are no longer registered as asset dependencies,DependencyLoadState/RecursiveDependencyLoadStatereport loaded while an import is still missing, andLoadedWithDependenciesfires early. Nothing in the engine relies on that behavior for shaders today, but it's a user-facing semantic getting quietly weaker.
Longer term, I think #10157 (comment) (asset packing with manifests) can address the root cause here: whether a path exists can only be determined by probing for it, and today probing means a request — on the web, a request that is expected to 404. With a manifest that becomes a local lookup, which makes load-time dependency discovery viable again, and makes the machinery above - initiating loads from the render world - unnecessary.
Thanks for reviewing this! Yeah, what you're saying makes a lot of sense. I think I'm going to close this PR because I agree that a manifest is what we really need for this to be fixed without introducing trade offs that fight the ECS mechanisms in place. |
Disclosure: I used Claude to assist with research into how to address this issue. Working with WESL and shader imports is new for me so I felt like it was justified to use it for assistance while I formulated a solution.
The initial version of this PR had some documenting comments that Claude assisted with because I did not feel confident about it. This was a mistake. I've since edited my comments by hand for clarity and brevity.
Objective
COLOR_MULTIPLIERis a const insidecustom_material_import.wesl, not a module beneath it. Rendering is fine, but the error is indistinguishable from a broken asset and fires for every item import in every WESL shader.import a::b::Cis ambiguous:Cmay be a filea/b/C.weslor a declaration insidea/b.wesl, and the import statement can't distinguish them.scan_wesl_importsemitted both readings as candidates, andAssetLoader::loadchose between them by reading each from storage and keeping whichever succeeded — so one read per import was designed to miss.When Bevy apps run natively, that's a silent failed stat; on the web, it surfaces in the Network tab of the Developer tools as an HTTP GET, and the browser logs the 404 before the response reaches Rust, so we can't suppress it.
Solution
Resolve ambiguities with wesl imports during compile time instead of guessing during run time.
weslresolves imports at the point of use, where the spec makes them unambiguous, so it only requests modules that exist — and it reports a module it can't find as structured data (ResolveError::ModuleNotFound(ModulePath, _)).ShaderCachekeeps the reportedModulePathinstead of discarding it.is_module_not_foundalready matched that exact variant; it now returns the path, carried onShaderImportNotYetAvailable.PipelineCacheloads it, filtered by origin.PathOrigin::Absoluteis a file under assets/; a package origin is an engine shader embedded viaload_shader_library!` and must never be fetched, or we'd reintroduce the same failing request for a different path.ShaderImport, since discovery re-reports it on every retry until the asset lands. The stored handle keeps it alive, asShader::file_dependenciesis no longer populated for WESL.AssetServer::load_state. An unresolved module is now the normal discovery signal and logs atdebug, so without this a typo'd import would fail silently.Testing
Browser (Chrome, WebGPU) —
shader_materialbuilt withcargo r -p build-wasm-example -- shader_material --api webgpu:custom_material_import/COLOR_MULTIPLIER.weslcustom_material.weslcustom_material_import.weslUnit tests —
cargo test -p bevy_shader. These three unit tests are new, each confirmed to fail when the behavior it covers is broken:item_import_names_only_its_parent_module— reconstructs the URL from the issue and asserts it's absent.missing_import_names_the_module_to_load— the unresolved module reaches the caller. Uses the existing fixture, whose root reachesbevy_render::mathsthrough an inline path with no import statement — a case no scan of import statements could find.module_path_origin_decides_whether_it_is_fetchable— pins the origin mapping. Breaking it yields Some(AssetPath("/lighting"))instead ofSome(Custom(..))`, which is the request that would 404.Reproducing, before and after:
Open
http://localhost:8801/index.htmland keep the tab focused — a throttled background tab pauses the frame loop that drives discovery, so requests stall and the page looks broken. The server's access log is the easiest place to read the result:main: two404s forcustom_material_import/COLOR_MULTIPLIER.weslcustom_material_import.weslfetched onceThe cube should render with the bird texture darkened by the imported constant's
0.5alpha in both cases.The two
*.wesl.meta404s are unrelated: they appear for any asset in any Bevy web build and are a handledNotFoundbranch.Platforms: web (WebGPU, Chrome) and native (macOS/Metal).
Notes for reviewers
PipelineCacheis in the render world and now initiates asset loads, where the render world normally only extracts from the main world.AssetServeris already a render world resource so it works, but it's the part I'm least sure about. I prototyped main-world discovery and abandoned it: shader defs live in the render world, so a main-world scan under-reports dependencies reachable only under an@ifflag — failing in the dangerous direction.weslreports only the first unresolved module per compile, so an N-deep chain takes N rounds. Imperceptible at the depths here, but linear in chain depth.wesl_module_requestsis never pruned. Bounded by distinct modules, so it can't grow without limit, but entries outlive their shaders. Happy to add cleanup onAssetEvent::Removedif that is appropriate. I wanted to keep this PR as minimal as possible.Not tested