feat(graphic): support local and file-based textures in material setup#435
Conversation
Updated material creation logic to allow using textures from local resources as well as files. The example now demonstrates creating materials from a file, a local texture, and without a texture. OnMaterialCreation now checks for both file existence and local texture container before assigning the texture.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntroduces local texture registration in the example, reorders cube setup and adds an untextured cube, changes G-buffer material uniform from emission to ambient, and updates GPU material creation to prefer loading a texture file, otherwise reuse a texture from the TextureContainer or emit a warning if missing. Changes
Sequence Diagram(s)sequenceDiagram
participant MaterialCreator as Material Creation
participant FS as File System
participant TC as TextureContainer
participant GPU as GPU/BindGroup
MaterialCreator->>FS: check ambient texture file exists?
alt file exists
FS->>TC: load texture & register (new textureId)
TC-->>MaterialCreator: textureId
MaterialCreator->>GPU: set GPUMaterial.texture = textureId
else file missing
MaterialCreator->>TC: query textureId by name
alt texture found in TC
TC-->>MaterialCreator: existing textureId
MaterialCreator->>GPU: set GPUMaterial.texture = textureId
else not found
MaterialCreator->>MaterialCreator: if ambient name non-empty → warn "texture not found"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.cpp`:
- Around line 30-40: The code path in OnMaterialCreation.cpp that checks
material.ambientTexName and textureContainer.Contains(textureId) can leave
GPUMaterial.texture uninitialized when the file doesn't exist and the texture
isn't pre-registered; update the logic to set a safe fallback: when
std::filesystem::exists(material.ambientTexName) is false and
textureContainer.Contains(textureId) is false (or when ambientTexName is empty),
assign GPUMaterial.texture to a known defaultTextureId (or a sentinel) and/or
register a default placeholder texture via
textureContainer.Add(defaultTextureId, ...) and ensure the bind group creation
that references textureId uses that defaultTextureId (or conditionally omits the
texture) and emits a warning via logging to aid debugging.
🧹 Nitpick comments (1)
examples/graphic_material_usage/src/main.cpp (1)
47-58: Minor naming inconsistency between texture's internal name and registration key.The texture is created with internal name
"LocalTexture"(line 52) but registered under key"LocalTextureName"(line 54), which matches the material'sambientTexName(line 49). While functionally correct, aligning these names would improve clarity for readers of this example.Suggested alignment
Object::Component::Material materialFromLocalTexture; materialFromLocalTexture.ambientTexName = "LocalTextureName"; auto &textureManager = core.GetResource<Graphic::Resource::TextureContainer>(); auto texture = - Graphic::Resource::Texture(core.GetResource<Graphic::Resource::Context>(), "LocalTexture", glm::uvec2(255, 255), + Graphic::Resource::Texture(core.GetResource<Graphic::Resource::Context>(), "LocalTextureName", glm::uvec2(255, 255), [](glm::uvec2 pos) { return glm::u8vec4(pos.x, pos.y, 0, 255); }); textureManager.Add("LocalTextureName", std::move(texture));
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin/default-pipeline/src/system/GPUComponentManagement/OnMaterialCreation.cpp (1)
62-70: Bind group unconditionally includes texture entry regardless of whether texture was loaded.The bind group creation at line 68 always includes
textureIdas a texture asset. If the texture loading failed (file not found and not in container) orambientTexNamewas empty,textureIdwon't exist in the container, potentially causing a runtime error when the bind group is used.Consider either:
- Ensuring a default texture is always registered, or
- Conditionally including the texture entry based on whether
GPUMaterial.texturewas assigned.
|
1 participant
This pull request enhances the material and texture handling logic in both the example usage and the GPU material creation system. The changes improve support for custom materials created from files, locally generated textures, and materials without textures. The most important changes are grouped below:
Material and Texture Creation Logic Improvements
main.cppnow demonstrates three types of material usage: materials loaded from file textures, materials created from locally generated textures, and materials without textures. It adds logic to generate a local texture programmatically and register it with the texture manager, and updates entity transforms for clarity.GPU Material System Robustness
OnMaterialCreation.cppnow first checks if the texture file exists and loads it if available. If the file does not exist but the texture is already present in the texture container (e.g., from a locally generated texture), it uses the existing texture instead of logging a warning or failing. This allows for more flexible material definitions and avoids unnecessary warnings.Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.