feat(scan): budget rules for triangles, bones, and textures (#365) - #712
Conversation
Introduce platform-profile and YAML budget rules (max triangles, bones, submeshes, draw calls, texture POT/format checks) with evaluateRules, SARIF metadata, CLI overrides, and example-budget profile. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 12 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements budget rule validation for mesh scans, adding configuration fields and rule-evaluation logic to enforce limits on triangle counts, bone counts, texture properties, and draw-call estimates. Changes include new ScanConfig fields, platform profile support, CLI budget override flags, and comprehensive rule evaluation in ScanEngine with tests. ChangesBudget Rules Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82c3c85f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (r.contains("max_texture_dimension")) | ||
| maxTextureResolution = r["max_texture_dimension"].toInt(); |
There was a problem hiding this comment.
When profile inheritance or another layered config leaves both keys in the merged rules map, this unconditional alias assignment makes max_texture_dimension win even if it came from a parent profile and the child overrode the canonical max_texture_resolution. In that mixed-name scenario, the child profile's texture budget is silently ignored; the alias should be normalized per layer or only applied when the canonical key is absent in the same override set.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
4288-4315:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--max-acmrloses CLI precedence when scope rules are present
maxAcmrOverrideis applied toconfig.maxAcmr(Line 4258), butcliRuleOverridesnever includes"max_acmr". That means scoped rules can still override the CLI value, which violates the precedence contract in this block.Suggested fix
if (maxVerticesOverride >= 0) cliRuleOverrides["max_vertex_count"] = config.maxVertexCount; if (minVerticesOverride >= 0) cliRuleOverrides["min_vertex_count"] = config.minVertexCount; + if (maxAcmrOverride >= 0.0) cliRuleOverrides["max_acmr"] = config.maxAcmr; if (maxTrianglesOverride >= 0) cliRuleOverrides["max_triangle_count"] = config.maxTriangleCount; if (maxTrianglesPerMeshOverride >= 0) cliRuleOverrides["max_triangles_per_mesh"] = config.maxTrianglesPerMesh;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CLIPipeline.cpp` around lines 4288 - 4315, The CLI flag maxAcmrOverride is applied to config.maxAcmr but never added to cliRuleOverrides, so scoped rules can override the CLI value; update the block that builds cliRuleOverrides (the same section using hasAllowedFormatsOverride and others) to add cliRuleOverrides["max_acmr"] = config.maxAcmr when maxAcmrOverride indicates the CLI override is present (mirror the existing pattern used for maxFileSizeMbOverride/maxTrianglesOverride), ensuring maxAcmrOverride, config.maxAcmr, and cliRuleOverrides are referenced consistently so the CLI value takes precedence.
🧹 Nitpick comments (1)
src/PlatformProfile_test.cpp (1)
208-217: ⚡ Quick winExpand assertions to cover all budget keys declared by the profile.
This test validates only part of
example-budget(max_triangle_count,max_bones, POT/probe). Please also assertmax_triangles_per_mesh,max_submesh_count,max_draw_calls,max_texture_dimensionmapping, andallowed_texture_formatsso future key drift is caught in one place.Suggested assertion additions
TEST(PlatformProfileLoaderTest, BuiltinExampleBudgetProfileLoadsRules) { const PlatformProfileLoadResult loaded = PlatformProfileLoader::load(QStringLiteral("example-budget")); ASSERT_TRUE(loaded.ok) << loaded.error.toStdString(); EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_triangle_count")).toInt(), 50000); + EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_triangles_per_mesh")).toInt(), 20000); EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_bones")).toInt(), 64); + EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_submesh_count")).toInt(), 8); + EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_draw_calls")).toInt(), 16); + EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("max_texture_dimension")).toInt(), 512); + EXPECT_EQ(loaded.profile.rules.value(QStringLiteral("allowed_texture_formats")).toStringList(), + (QStringList{QStringLiteral("png"), QStringLiteral("jpg")})); EXPECT_TRUE(loaded.profile.metadata.value(QStringLiteral("inspect_textures")).toBool()); ScanConfig config = ScanConfig::defaults(); applyPlatformProfile(config, loaded.profile); EXPECT_EQ(config.maxTriangleCount, 50000); + EXPECT_EQ(config.maxTrianglesPerMesh, 20000); EXPECT_EQ(config.maxBoneCount, 64); + EXPECT_EQ(config.maxSubmeshCount, 8); + EXPECT_EQ(config.maxDrawCalls, 16); + EXPECT_EQ(config.maxTextureResolution, 512); + EXPECT_EQ(config.allowedTextureFormats, (QStringList{QStringLiteral("png"), QStringLiteral("jpg")})); EXPECT_TRUE(config.probeTextureFiles); EXPECT_TRUE(config.requireTexturePowerOfTwo); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/PlatformProfile_test.cpp` around lines 208 - 217, Extend the test assertions after applyPlatformProfile to verify all budget keys from the profile: add EXPECT_EQ checks that loaded.profile.rules contains and maps max_triangles_per_mesh -> config.maxTrianglesPerMesh, max_submesh_count -> config.maxSubmeshCount, max_draw_calls -> config.maxDrawCalls, max_texture_dimension -> config.maxTextureDimension, and that allowed_texture_formats maps to config.allowedTextureFormats (or the corresponding ScanConfig member) and contains the expected formats; keep references to ScanConfig, applyPlatformProfile, and loaded.profile to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 4288-4315: The CLI flag maxAcmrOverride is applied to
config.maxAcmr but never added to cliRuleOverrides, so scoped rules can override
the CLI value; update the block that builds cliRuleOverrides (the same section
using hasAllowedFormatsOverride and others) to add cliRuleOverrides["max_acmr"]
= config.maxAcmr when maxAcmrOverride indicates the CLI override is present
(mirror the existing pattern used for
maxFileSizeMbOverride/maxTrianglesOverride), ensuring maxAcmrOverride,
config.maxAcmr, and cliRuleOverrides are referenced consistently so the CLI
value takes precedence.
---
Nitpick comments:
In `@src/PlatformProfile_test.cpp`:
- Around line 208-217: Extend the test assertions after applyPlatformProfile to
verify all budget keys from the profile: add EXPECT_EQ checks that
loaded.profile.rules contains and maps max_triangles_per_mesh ->
config.maxTrianglesPerMesh, max_submesh_count -> config.maxSubmeshCount,
max_draw_calls -> config.maxDrawCalls, max_texture_dimension ->
config.maxTextureDimension, and that allowed_texture_formats maps to
config.allowedTextureFormats (or the corresponding ScanConfig member) and
contains the expected formats; keep references to ScanConfig,
applyPlatformProfile, and loaded.profile to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04d983f2-4ea8-4b83-957e-6f565d4d5ec9
📒 Files selected for processing (9)
profiles/example-budget.jsonsrc/CLIPipeline.cppsrc/PlatformProfile.cppsrc/PlatformProfile_test.cppsrc/ScanConfig.cppsrc/ScanConfig.hsrc/ScanConfig_test.cppsrc/ScanEngine.cppsrc/ScanEngine_test.cpp
Prefer max_texture_resolution over the dimension alias when both keys appear in the same override set, and ensure --max-acmr is included in CLI scope overrides so scoped rules cannot override it. Co-authored-by: Cursor <cursoragent@cursor.com>
Assert all budget rule keys from the profile map through applyPlatformProfile into ScanConfig. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
AssetInfofields:max_triangle_count,max_triangles_per_mesh,max_bones,max_submesh_count,max_draw_calls, texture POT/format checks, andmax_texture_dimensionalias for texture size limits.ScanConfig/evaluateRules, SARIF descriptions, CLI overrides (--max-triangles,--max-bones, etc.), and built-inexample-budgetprofile.textureStats(enable via profile metadatainspect_textures: trueorexample-texture-inspect/example-budget).Test plan
ScanEngineTest.EvaluateRules_*budget rule coverage (syntheticAssetInfo)ScanEngineTest.FormatSarif_IncludesBudgetRuleDescriptionsScanConfigApplyOverridesTest.ApplyRuleOverridesAffectsAllFieldsPlatformProfileLoaderTest.BuiltinExampleBudgetProfileLoadsRulesCloses #365
Made with Cursor
Summary by CodeRabbit
Release Notes
--max-triangles,--max-bones,--max-submeshes,--max-draw-calls) for budget overrides.