Refactor API status handling and add occlusion culling - #95
Conversation
🌿 Roseau API Breaking Change Report
🔴 rendering — 5 breaking change(s)
|
Reason: script execution via -e/-c flag Reply |
|
⏱️ Rate limited. Waiting 2.9s (attempt 2/3)... |
|
⏱️ Rate limited. Waiting 4.9s (attempt 3/3)... |
|
❌ Rate limited after 3 retries — Concurrency limit exceeded for user, please retry later |
|
API call failed after 3 retries: Concurrency limit exceeded for user, please retry later |
|
分析完成。以下是审查意见。 代码审查摘要 — PR #95操作: synchronize 🔴 关键(合并前需修复)
|
| 声称 | 状态 | 对应文件 |
|---|---|---|
| OcclusionCuller/OcclusionKey/OcclusionMethod 抽象 | ✅ | optimization/occlusion/*.java |
| 查询渲染管线 + 顶点/片元着色器 + FullTransformsUbo | ✅ | ALRPipelines.OCCLUSION_QUERY, occlusion_query.vsh/.fsh |
| GPU 设备接口 + OpenGL samples-query(GlSamplesQuery 生命周期) | GlSamplesQuery 存在但含 2 个 🔴 bug | |
| 可复用 GPU 资源 + ring buffer 基础(Query 池 / Query 缓冲池) | ✅ | GpuReusableResource(RingBuffer), LoopResetPool, SimpleResetPool, QueryBufferPack(RingBuffer) |
| Query 帧状态管理(当前帧提交执行、下帧读结果) | ✅ | FrameState, GpuQueryOcclusionCuller |
| compute-pass debug-group 懒 Supplier | ✅ | ALRComputePass(+Backend), GlComputePassBackend, GlDeviceMixin |
| ApiStatus.Internal 注解调整 | ✅ | 15 个 mixin 移除 + 内部实现类补注 |
| [x] Samples Query backend | 已实现但 glEndQuery/越界读使其实质不可用 | |
| [ ] Hi-Z backend | ✅(按 TODO 未声称完成) | 仅存 stub + depth_convert/ffx_spd shader + SPD pipeline 注册,未接线 |
| [ ] 接入 vanilla SubmitNodeStorage | ✅(未声称完成) | 未实现,符合描述 |
结论: REQUEST_CHANGES — 基础设施和框架设计扎实,但 GlSamplesQuery 的两处错误(glEndQuery target 传参 + 4/8 字节越界读)会让已声称完成的 Samples Query 后端在接入时立即挂起渲染线程;修复后再合并。其余为 Hi-Z 接线前的潜在问题,建议顺手修正宽高顺序和默认 flag 映射。
由 Hermes Agent 审查
|
💾 Self-improvement review: Skill 'rendering-gpu-pr-review' created. |
|
所有文件已审查完毕。汇总分析结果,输出审查意见: 代码审查摘要 — PR #95 (synchronize)操作: synchronize 📋 声称验证表
🔴 关键(接入即错的 latent bug,建议合并前修)
|
3c58b50 to
4cdf68c
Compare
|
💾 Self-improvement review: Patched SKILL.md in skill 'rendering-gpu-pr-review' (1 replacement). |
Reason: script execution via -e/-c flag Reply |
🌿 Roseau API Breaking Change Report
🔴 rendering — 6 breaking change(s)
|
|
All verification complete. I have a full picture of the PR. Writing the review: 代码审查摘要 — PR #95操作: synchronize 🔴 关键(接入前必须修复)
|
| 声称 | 状态 | 对应文件 |
|---|---|---|
| OcclusionCuller / OcclusionKey / OcclusionMethod 抽象 | ✅ | 3 个新文件 |
| occlusion-query 管线 + 顶点/片元着色器 | ✅ | ALRPipelines.OCCLUSION_QUERY + vsh/fsh |
| GPU 设备接口扩展 + GlSamplesQuery | ✅ | ALRGpuDeviceExtension(+Backend)、GlDeviceMixin、GpuDeviceMixin、ALRHICapabilities |
| GPU 资源/ring-buffer 池基础 | ✅ | GpuReusableResource(Pool)、LoopResetPool、SimpleResetPool |
| Query 帧状态管理(本帧提交、次帧读结果) | ✅ | FrameState、GpuQueryOcclusionCuller、QueryInstance |
| pushDebugGroup 改 Supplier | ✅ | ALRComputePass 全链路 |
| ApiStatus.Internal 注解调整 | ✅ | 渲染 + sync 模块 |
| Samples Query 后端 | GpuQueryOcclusionCuller;ALROptimizations.getOcclusionCuller() 无调用者 |
|
| Hi-Z 后端 | HierarchicalZOcculusionCuller 空实现 + ffx_spd/depth_convert + UBOs;createInstance 返回 null |
|
| 集成 vanilla SubmitNodeStorage | ❌ TODO | 无对应代码 |
🧪 测试建议
| 被测目标 | 推荐测试场景 | 优先级 |
|---|---|---|
FrameState.runQueries/fetchResults |
连续多帧 key 增删:池无重复 acquire、结果延迟一帧生效、释放后复用安全 | 🔴 |
| UBO 布局(接入后) | IntSizeUbo/ConvertDepthParamsUbo 写入字节与 GLSL 声明逐字段对照 |
🔴 |
GpuQueryOcclusionCuller.shouldDraw |
相机在 AABB 内部 / 包围盒部分可见 / 完全被地形遮挡三场景 | 🟡 |
GlSamplesQuery.getValue |
查询未 begin/end 时读取、release 后结果重置、close 后抛异常 | 🟡 |
结论: REQUEST_CHANGES — 基础设施质量整体很高(GL 调用、池时序、帧状态设计都正确),但三个 🔴 均为确定性缺陷:两个 UBO 宽高互换是接入即现的错误、withCull(true) 在相机入盒场景会错误剔除、Hi-Z 占位桩默认返回 false 是隐患。三者修复成本都极低(各一行级别),建议修复后再合入,避免错误语义沉淀到后续接线 PR。ALROptimizations 的接线入口建议在本 PR 内补一个最小调用链(哪怕只在 debug 模式),让 latent 问题能在真实帧循环中提前暴露。
由 Hermes Agent 审查
|
💾 Self-improvement review: Patched SKILL.md in skill 'rendering-gpu-pr-review' (1 replacement). |
🌿 Roseau API Breaking Change Report
🔴 rendering — 9 breaking change(s)
|
|
审查完成,所有关键路径已交叉验证(本地克隆 AnvilLib@dev/26.1、MC 26.1 反编译产物、shader、GL 常量值)。以下是审查意见: 代码审查摘要 — PR #95操作: synchronize 🔴 关键(确定性代码缺陷,接入即触发)
|
| 声称 | 状态 | 对应文件 |
|---|---|---|
| OcclusionCuller/Key/Method 抽象 | ✅ | optimization/occlusion/* |
| 查询管线 + 顶点/片元 shader + FullTransformsUbo | ✅(含 🔴 绕序/cull 问题) | ALRPipelines, occlusion_query.*, FullTransformsUbo |
| GPU device 接口 + GL samples-query 封装 | ✅ | GlSamplesQuery, GpuQueryObject, GlDeviceMixin |
| 资源池 + ring buffer 基础 | ✅(含 |
GpuReusableResourcePool, LoopResetPool, SimpleResetPool |
| 帧状态管理(本帧提交/次帧读取) | ✅ | FrameState, GpuQueryOcclusionCuller |
| compute debug-group 惰性 Supplier | ✅ | ALRComputePass, GlComputePassBackend |
| ApiStatus.Internal 注解调整 | ✅ | 15 个 mixin 移除 + 内部类标注 |
| Hi-Z 后端 | HierarchicalZOcclusionCuller | |
| Samples Query 后端 | GpuQueryOcclusionCuller | |
| 接入 vanilla SubmitNodeStorage | ❌ TODO 未勾选(自认) | 无 |
🧪 测试建议
| 被测目标 | 推荐场景 | 优先级 |
|---|---|---|
ALRComputePass.bindAll |
首次调用即 IOOBE(当前必现) | 🔴 |
ImageArrayBinding.apply |
resource.size() == size 与 > size 两分支(当前逻辑颠倒) |
🔴 |
QueryInstance 立方体 |
从 +x/-x 各方向 + 相机在 AABB 内部观察(samples 是否 >0) | 🔴 |
GpuSampleQueryPool |
连续 3 帧 key 数 > 池容量,验证 expand 与 release 时序 | 🟡 |
ALROptimizations |
启动顺序(create 先于 compute init)下的后端选择断言 | 🟡 |
结论: REQUEST_CHANGES — 基础设施方向正确、多数问题因未接线而处于 latent;但 bindAll 是对既有 API 的确定性回归、x+ 面绕序与 IntSizeUbo 宽高互换是确定的代码缺陷,建议合并前修复;其余
由 Hermes Agent 审查
|
💾 Self-improvement review: Patched SKILL.md in skill 'rendering-gpu-pr-review' (1 replacement). |
Summary
This PR lays the first-stage foundation for occlusion culling in the rendering module and clarifies the visibility of selected internal APIs. It introduces the culler abstraction, GPU Query resources and draw path, and an OpenGL samples-query wrapper. The selectable Hi-Z and Query culling backends will be integrated in follow-up work.
Changes
OcclusionCuller,OcclusionKey, andOcclusionMethodto provide a common abstraction for rendering-feature submission, visibility decisions, and backend selection.FullTransformsUbo.GlSamplesQuerylifecycle wrapper.Supplier<String>values, avoiding unnecessary debug-text construction.ApiStatus.Internalannotations by removing them from mixin classes and explicitly marking rendering and sync classes, records, and extension interfaces that are internal implementations.Impact and Compatibility
ALRComputePass.pushDebugGroupand its backend implementation now acceptSupplier<String>instead ofString; direct implementations and callers of this internal API must be updated accordingly.TODO