WIP: Engine parity: 15 missing features (localization, accessibility, profiler, behaviour trees, rollback, streaming, weather, blend shapes, LOD...) #142
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "khyretos/claude/engine-parity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Requested by Kees
WIP: one big PR on purpose, to be split. Kees (10-27): build the missing GAPS.md features locally first, split into small PRs later. This is everything built so far in one commit on top of
main(4da51cf0). Nothing here should merge as is: the WIP prefix keeps auto-merge off. Each feature below is independent enough to cut out; the files they share aredocs/GAPS.md,docs/SCRIPTING.md,docs/README.md,mkdocs.yml,ROADMAP.md,engine/CMakeLists.txt,tests/CMakeLists.txt,ScriptModule.h/.cpp,ScriptBindings.cpp,ModelModule.h/.cppandEngineSettings.What it adds
kke::bidi(Arabic shaping, right-to-left); Noto Arabic and Hebrew fonts; Lualoc.*; engine menus in en, nl, de, fr, es, ar, hesubtitles.*), colour-blind modes (matrix in the tone-map pass), UI scale, high contrast, reduced motion. Still 🟡: no real screen readerkke::crash): signal or exception, backtrace, last log lines. Sending is off by default (privacy.sendCrashReports). POSIX only, so 🟡bt.*loadModel("x.bvh")gives a skeleton and a clip for the retargeterkke::Timeline: keyframe tracks, events, markers, spans, clips, nested timelines; Luatimeline.*music.*world.*weather.*kke::FaceRig(expressions, blinking, lip-sync), Luaface.*graphics.autoLod,graphics.lodBias,KKE_LOD=0Limits, stated plainly
Tests
kke_tests: all pass (1649 in the last full run).python3 -W error tools/ci/check_{dependencies,game_modules,std_includes}.pypass.tools/docs_site/run_recipes.py(39 ok,playskipped) andtools/check_game starter_game --seconds 5pass under Xvfb on lavapipe.Title
WIP: Engine parity: 15 missing features (localization, accessibility, profiler, behaviour trees, rollback, streaming, weather, blend shapes, LOD...)
User description
Requested by Kees
WIP: one big PR on purpose, to be split. Kees (10-27): build the missing GAPS.md features locally first, split into small PRs later. This is everything built so far in one commit on top of
main(4da51cf0). Nothing here should merge as is: the WIP prefix keeps auto-merge off. Each feature below is independent enough to cut out; the files they share aredocs/GAPS.md,docs/SCRIPTING.md,docs/README.md,mkdocs.yml,ROADMAP.md,engine/CMakeLists.txt,tests/CMakeLists.txt,ScriptModule.h/.cpp,ScriptBindings.cpp,ModelModule.h/.cppandEngineSettings.What it adds
kke::bidi(Arabic shaping, right-to-left); Noto Arabic and Hebrew fonts; Lualoc.*; engine menus in en, nl, de, fr, es, ar, hesubtitles.*), colour-blind modes (matrix in the tone-map pass), UI scale, high contrast, reduced motion. Still 🟡: no real screen readerkke::crash): signal or exception, backtrace, last log lines. Sending is off by default (privacy.sendCrashReports). POSIX only, so 🟡bt.*loadModel("x.bvh")gives a skeleton and a clip for the retargeterkke::Timeline: keyframe tracks, events, markers, spans, clips, nested timelines; Luatimeline.*music.*world.*weather.*kke::FaceRig(expressions, blinking, lip-sync), Luaface.*graphics.autoLod,graphics.lodBias,KKE_LOD=0Limits, stated plainly
Tests
kke_tests: all pass (1649 in the last full run).python3 -W error tools/ci/check_{dependencies,game_modules,std_includes}.pypass.tools/docs_site/run_recipes.py(39 ok,playskipped) andtools/check_game starter_game --seconds 5pass under Xvfb on lavapipe.PR Type
Enhancement, Tests
Description
Adds localization, accessibility, and profiler systems
Implements behaviour trees, timelines, and adaptive music
Introduces world streaming, weather, and rollback netcode
Includes LOD generation, mocap import, and crash reporting
Diagram Walkthrough
File Walkthrough
21 files
Implements full behaviour tree engine with nodes, blackboard and JSONparsingImplements timeline system with tracks, events, clips and nestedtimelinesAdds localization with JSON tables, CLDR plurals and RTL supportBinds LOD, facial animation and adaptive music to LuaAdds motion blur request flag for accessibility settingsAdaptive music director with segments and stingersRight-to-left text shaping and bidi algorithmAdded vertex buffer accessor for LOD supportLua bindings for behaviour trees with blackboard accessBVH mocap import and motion cleaning utilitiesAutomatic LOD, blend shapes and facial animation supportAnimation compression with quantization and key reductionPOSIX crash reporting with backtraces and opt-in uploadImplements weather system with presets, transitions, and updatesAdds rollback netcode for deterministic simulation and desyncdetectionAdds Lua bindings for timeline creation, playback, and event deliveryExtends post-pass push constants to support color-blind correctionrowsImplementation of runtime profiler with capture supportStreaming world implementation with cell loadingFace rig system with blend shapes and lip syncWeather system with particles and lighting effects14 files
Tests adaptive music director with scheduled voices and transitionsTests Lua weather system including rain, snow, lighting and hooksAdds comprehensive unit tests for timeline evaluation and playbackComprehensive behavior tree unit tests covering all node types andfactoryRollback netcode tests for prediction, lag compensation, and desyncdetectionWorld streaming tests for cell loading, unloading, and origin shiftingLua scripting tests for world streaming hooks and cell managementfunctionsBVH mocap import and motion cleaning testsWeather system simulation and event testsTests animation compression quality, quantization, and fileround-tripsTests localization loading, plurals, RTL, numbers, and fallback chainsTests for ScriptTimeline Lua bindings and featuresTests for behaviour tree Lua scripting systemTests for automatic mesh LOD generation algorithm1 files
Reduced motion check for camera shake accessibility92 files
PR Reviewer Guide 🔍
📋 Focus areas: 🔴 12 open · ✅ 0 handled. Every push re-checks them; a handled area moves to Resolved findings at the bottom.
Potential Integer Overflow in Parallel Node
In the
Parallelnode'sonTickmethod, the code sums up success and failure counts from child nodes intookandbadvariables. If a behavior tree is constructed with a very large number of children (exceeding the maximum value ofsize_t), this summation could overflow, leading to incorrect status determination (e.g., reporting success when it should fail). While unlikely in typical use cases, robustness suggests using a type that can safely hold the sum or checking for potential overflow if the number of children is expected to be large.Hard-coded Audio Sample Rate
The test file defines a constant
kRate = 48000for the audio sample rate. This hard-codes the test environment to a specific sample rate, which may not match the actual runtime configuration of the engine or other tests. If the engine supports configurable sample rates, these tests might fail or produce incorrect timing results on systems configured differently. It would be better to either use a default value from the engine's configuration or explicitly document why 48kHz is chosen as the test standard.Missing Error Handling for File I/O
The
loadFilefunction reads a file into a string stream and then passes it toloadJson. If the file is empty or contains only whitespace,nlohmann::json::parsewill throw an exception. While the exception is caught, the error message returned to the caller will be "not JSON: [exception message]" instead of a more specific "file is empty" or similar. This makes debugging localization file issues harder. Additionally, if the file path is invalid, the error message is just "can't read ", which is fine, but the combination of these cases could lead to confusing logs.Hardcoded Test Data in Source
The test
ABossFightWithPhases(line 416) uses a large JSON string literal directly in the source code. While this is common for small configs, it makes the test harder to read and maintain compared to loading from a separate file or using a helper function. It also increases the risk of typos in the JSON syntax within the C++ source.Potential Integer Overflow in Random Sequence
In the
RandomSequenceRunsEveryChildOncetest (line 244), aBehaviorTreeis created with a seed of3. ThebtRandomSequenceconstructor likely uses this seed to initialize a random number generator. If the implementation ofbtRandomSequencedoes not properly handle the seed or if the internal state machine relies on integer arithmetic that can overflow given the number of children (4 in this case), it might produce incorrect ordering or fail to visit all children. While unlikely with modern RNGs, it's worth noting that relying on a hardcoded small seed for a test that verifies randomness properties is slightly risky; a larger variety of seeds or a check against multiple runs would be more robust.Missing Unicode Normalization
The
decodefunction in Bidi.cpp handles UTF-8 decoding but does not perform Unicode normalization. This can lead to inconsistent text representation when combining characters (like Arabic or Hebrew) are involved, potentially causing display issues or incorrect bidirectional algorithm behavior. For robust localization support, normalization should be applied before processing.Incomplete Arabic Ligature Support
The
lamAleffunction only handles specific alef characters (0x622, 0x623, 0x625, 0x627) for lam-alef ligatures. However, Arabic script includes other forms of alef that may require ligature handling depending on context. This incomplete coverage could result in incorrect shaping for certain Arabic words containing lam followed by different alef variants.Potential Resource Leak in Morph Parts Cleanup
In the
removemethod, morph parts are retired usingstd::make_shared<std::vector<MorphPart>>(std::move(inst.morphParts)). While this moves the data, ifinst.morphPartscontains shared pointers to GPU buffers or other resources that need explicit cleanup beyond just moving the vector, there might be a leak. Ensure all underlying resources held byMorphPartare properly released when the instance is destroyed.Potential Integer Overflow
In
Weather::update, the wind speed calculation multipliess.wind(up to 100) by(1.0f + s.gust * n). Sincencan reach approximately 1.0, the result can be around 200 m/s. While this is within float range, ifs.windors.gustwere to increase further in future modifications, or if the logic changes, this could lead to unrealistic physics or potential overflow in downstream calculations that expect reasonable wind speeds. The current bounds check forwind(0 to 100) is applied at definition time but not enforced during runtime updates where gusts are applied.Missing Validation for Random Weather Parameters
The
randomWeatherfunction acceptsminSecondsandmaxSecondsparameters but does not validate them against the hardcoded defaults or ensure they are positive. If a user passes negative values or zero, the logicstd::max(1.0f, std::isfinite(minSeconds) ? minSeconds : 120.0f)might behave unexpectedly ifminSecondsis negative (it would clamp to 1.0, which is fine, but the intent might be violated). More critically, ifminSecondsis NaN or infinite, the behavior is undefined due tostd::isfinitechecks only handling finite numbers, potentially leading to infinite loops or incorrect timing ifm_holdbecomes invalid.Potential Out-of-Bounds Access in cleanMotion
In
cleanMotion, the loopfor (size_t i = 1; i + 1 < f.size(); ++i)accessesf[i - 1],f[i], andf[i + 1]. This is safe as long asf.size() >= 3. However, the conditionif ((o.spikeDegrees > 0.0f || o.spikeMetres > 0.0f) && f.size() > 2)ensuresf.size()is at least 3 before entering the loop. But inside the smoothing section, there is a similar checkif (o.smoothSeconds > 0.0f && f.size() > 2). Iff.size()is exactly 2, the loopfor (int i = 0; i < n; ++i)wheren = static_cast<int>(f.size())will run fori=0andi=1. Inside, it accessessrc[static_cast<size_t>(j)][b]wherejranges from-radiustoradius. Ifradiusis greater than 0 (which it is ifsigma > 0), thenjcan be negative or exceedn-1, causing out-of-bounds access onsrc.Potential Division by Zero in reduceKeys
In
reduceKeys, the lineconst float t = static_cast<float>(i - a) / static_cast<float>(b - a);calculates a normalized time. Ifaandbare equal (which shouldn't happen given the stack logicif (at - a > 1) stack.emplace_back(a, at); if (b - at > 1) stack.emplace_back(at, b);), it would cause division by zero. However, the initial stack pushstack.emplace_back(0, n - 1)ensuresb - aisn - 1. Ifn == 1, the function returns early. Ifn == 2, the loopfor (size_t i = a + 1; i < b; ++i)doesn't execute, soworstremains 0.0f, and no new keys are added. The division only happens ifiiterates, implyingb - a > 1. Thus, this is likely safe, but the comment "Ramer-Douglas-Peucker over time" suggests a standard algorithm whereaandbare distinct indices. The code seems robust here, but it's worth noting that ifb - awere somehow 0 due to a bug in stack management, it would crash.ℹ️ Chunked review: the diff exceeded the model token budget, so it was reviewed in 8 chunks and the per-chunk results were merged.
⚠️ Review coverage: The following files were not included in this review because of the token budget:
engine/src/FaceRig.cppengine/src/ScriptWeather.cpptests/test_script_behavior.cpptests/test_script_music.cpptests/test_profiler.cppengine/src/WorldStream.cppengine/src/CrashReporter.cpptests/test_morph.cppengine/src/Application.cppengine/src/modules/ScriptLocale.cpptests/test_accessibility.cpptests/test_face_rig.cppengine/src/modules/GameShellModule.cppengine/src/modules/UiModule.cpptests/test_lod_chain.cppengine/src/EngineSettings.cpptests/test_crash_reporter.cppengine/src/Accessibility.cppengine/src/ScriptMusic.cppengine/src/AudioMixer.cppengine/src/modules/ScriptBindings.cpptests/test_bidi.cppengine/src/modules/ScriptProfile.cppengine/src/Log.cppengine/src/Animator.cpptests/test_engine_settings.cppengine/src/GltfAsset.cppengine/src/Morph.cppengine/src/modules/ScriptModule.cppengine/src/Subtitles.cppengine/src/Renderer.cppengine/src/LodChain.cppengine/src/modules/StatsModule.cppengine/src/modules/SettingsModule.cppengine/src/ModelAsset.cppengine/src/CameraDirector.cppshaders/tonemap_post.fragtools/morph_test_model.pyengine/include/kke/ai/BehaviorTree.hengine/include/kke/Timeline.hengine/include/kke/MusicDirector.hengine/include/kke/Rollback.hengine/include/kke/modules/ModelModule.hengine/include/kke/Profiler.hengine/include/kke/WorldStream.hengine/include/kke/Weather.hengine/include/kke/Localization.hengine/include/kke/AnimCompress.hengine/include/kke/FaceRig.hengine/include/kke/Mocap.h... and 52 more
PR Code Suggestions ✨
No code suggestions found in the successfully analyzed chunks.
⚠️ Suggestion coverage: 102 file(s) were not analyzed because of the token budget or maximum chunk calls:
engine/src/FaceRig.cpp,engine/src/ScriptWeather.cpp,tests/test_script_behavior.cpp,tests/test_script_music.cpp,tests/test_profiler.cpp,engine/src/WorldStream.cpp,engine/src/CrashReporter.cpp,tests/test_morph.cpp,engine/src/Application.cpp,engine/src/modules/ScriptLocale.cpp,tests/test_accessibility.cpp,tests/test_face_rig.cpp,engine/src/modules/GameShellModule.cpp,engine/src/modules/UiModule.cpp,tests/test_lod_chain.cpp,engine/src/EngineSettings.cpp,tests/test_crash_reporter.cpp,engine/src/Accessibility.cpp,engine/src/ScriptMusic.cpp,engine/src/AudioMixer.cpp,engine/src/modules/ScriptBindings.cpp,tests/test_bidi.cpp,engine/src/modules/ScriptProfile.cpp,engine/src/Log.cpp,engine/src/Animator.cpp,tests/test_engine_settings.cpp,engine/src/GltfAsset.cpp,engine/src/Morph.cpp,engine/src/modules/ScriptModule.cpp,engine/src/Subtitles.cpp,engine/src/Renderer.cpp,engine/src/LodChain.cpp,engine/src/modules/StatsModule.cpp,engine/src/modules/SettingsModule.cpp,engine/src/ModelAsset.cpp,engine/src/CameraDirector.cpp,shaders/tonemap_post.frag,tools/morph_test_model.py,engine/include/kke/ai/BehaviorTree.h,engine/include/kke/Timeline.h,engine/include/kke/MusicDirector.h,engine/include/kke/Rollback.h,engine/include/kke/modules/ModelModule.h,engine/include/kke/Profiler.h,engine/include/kke/WorldStream.h,engine/include/kke/Weather.h,engine/include/kke/Localization.h,engine/include/kke/AnimCompress.h,engine/include/kke/FaceRig.h,engine/include/kke/Mocap.h, and 52 more.🧹 Lint report · ❌ 44 problems in 6 checks · 🧹 3 formatting checks to tidy
Nothing here blocks the merge (report only).
MARKDOWN · markdownlint: first lines of the output
CPP · cppcheck: first lines of the output
YAML · yamllint: first lines of the output
BASH · shellcheck: first lines of the output
DOCKERFILE · hadolint: first lines of the output
REPOSITORY · trivy: first lines of the output
🧹 Formatting only (layout, no behaviour; the nightly lint-fix job applies these as a PR): YAML · prettier (57 files checked), BASH · shfmt (11 files checked), JSON · prettier (154 files checked)
✅ Clean: actionlint, git_diff, gitleaks, grype, jsonlint, secretlint, stylelint, trufflehog, xmllint
MegaLinter on soucouyant · full logs in the Actions run · comment
/improvefor AI fix suggestionsView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.