Conversation
…load and cyclic testing
- clean all cpu accelerate tasks before loading
- emmsive
- animated default - auto camera fix - materials max t o16384
- prevent physic kick
Summary of ChangesHello @gameknife, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the engine's glTF loading capabilities by introducing a comprehensive robustness testing framework and improving compatibility with various glTF asset features, including WebP textures and emissive materials. It also refines the integration between animation and the physics system, ensuring animated objects behave correctly. Additionally, the changes standardize development workflows through updated documentation and build configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant set of features and improvements, primarily focused on enhancing glTF support and rendering capabilities. Key additions include a glTF robustness test runner that automatically downloads and tests models from the Khronos sample repository, support for WebP textures, and handling of emissive textures in the glTF loader and shaders.
The review has identified a critical issue in the emissive material loading logic where emissive strength is not being applied, a high-severity issue in the new glTF test runner that could cause it to hang, and several medium-severity issues related to code style, potential robustness improvements in camera calculations, and minor typos. Overall, this is a valuable contribution that significantly expands the engine's capabilities, and addressing the feedback will improve its correctness and robustness.
| // TODO: may impl per pixel mat type | ||
| if (emissive != mat.extensions.end()) | ||
| { | ||
| float power = static_cast<float>(emissive->second.Get("emissiveStrength").GetNumberAsDouble()); |
There was a problem hiding this comment.
The power variable, extracted from the KHR_materials_emissive_strength extension, is calculated but never used. The previous implementation used this to scale the emissive color and set the material type to DiffuseLight. This change appears to have broken emissive material handling for materials with emissive strength, as the material's emissive properties are not being correctly applied.
| case State::Loading: | ||
| if (engine_->GetEngineStatus() == NextRenderer::EApplicationStatus::Running) | ||
| { | ||
| renderTimer_ = 0.0f; | ||
| currentState_ = State::Rendering; | ||
| } | ||
| break; |
There was a problem hiding this comment.
The test runner's Loading state waits for the engine status to become Running. If a glTF file is valid but causes the engine to crash or hang during loading, the test runner will be stuck in this state indefinitely. It would be more robust to add a timeout to the Loading state. If the engine doesn't become Running within a certain time, the runner should log an error, skip the current model, and proceed to the next one.
| public float Reserverd2; | ||
| public float Reserverd3; |
| if (model.textures[texture].extensions.find("EXT_texture_webp") != model.textures[texture].extensions.end()) | ||
| { | ||
| if (model.textures[texture].extensions["EXT_texture_webp"].Has("source")) | ||
| { | ||
| imageIdx = model.textures[texture].extensions["EXT_texture_webp"].Get("source").GetNumberAsInt(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for handling the EXT_texture_webp extension can be made more efficient and readable by avoiding repeated lookups. Consider using C++17's if with initializer or storing the result of find to avoid searching the map twice. A similar pattern is repeated on lines 383-389.
if (const auto it = model.textures[texture].extensions.find("EXT_texture_webp"); it != model.textures[texture].extensions.end()) {
if (it->second.Has("source"))
{
imageIdx = it->second.Get("source").GetNumberAsInt();
}
}| newCamera.NearPlane = glm::min( radius * 0.5f, 0.2f); | ||
| newCamera.FarPlane = glm::min( radius * 100.f, 1000.f); |
There was a problem hiding this comment.
The calculation for NearPlane and FarPlane might not be robust for all scene sizes. For example, newCamera.NearPlane = glm::min( radius * 0.5f, 0.2f); will cap the near plane at a maximum of 0.2, which could be too small for very large scenes, potentially causing precision issues. Similarly, FarPlane is capped at 1000. Consider a more dynamic calculation, for example newCamera.NearPlane = std::max(0.01f, radius * 0.01f); and newCamera.FarPlane = radius * 2.0f; to better adapt to scenes of varying scales.
| float Reserverd2; | ||
| float Reserverd3; |
There was a problem hiding this comment.
| if (track.Time_ > durationMax) | ||
| { | ||
| track.Time_ = 0; | ||
| track.PlaySpeed_ = -1.0f; | ||
| } | ||
| if ( track.Time_ < 0.0f ) | ||
| { | ||
| track.PlaySpeed_ = 1.0f; | ||
| } |
There was a problem hiding this comment.
| task.GetContext(taskContext); | ||
| textureImages_[taskContext.textureId]->MainThreadPostLoading(mainThreadCommandPool_); | ||
| //SPDLOG_INFO("{}", taskContext.outputInfo.data()); | ||
| SPDLOG_INFO("{}", taskContext.outputInfo.data()); |
There was a problem hiding this comment.
This SPDLOG_INFO log seems to have been uncommented, possibly for debugging. In a production or final build, this could lead to verbose logging which might be undesirable. Consider changing it to SPDLOG_DEBUG or removing it if it's no longer needed.
| SPDLOG_INFO("{}", taskContext.outputInfo.data()); | |
| //SPDLOG_INFO("{}", taskContext.outputInfo.data()); |
No description provided.