perf(scene): split PrefabComponent/TagComponent/AudioSourceComponent into hot/cold halves#578
Conversation
…into hot/cold halves Moves editor- and config-only data out of three per-instance ECS components to shrink their hot-array footprint (issue #444): - PrefabComponent: the three unordered_set<string> override-tracking sets move behind a lazily-allocated PrefabOverrideSets (null when a prefab instance has no overrides, the common case). 184-208 bytes -> 24 bytes. - TagComponent: the transient `renaming` editor flag moves to SceneHierarchyPanel::m_RenamingEntityUUID (panel-local rename state, since only one entity can be mid-rename at a time). - AudioSourceComponent: playback config, event wiring, and the SoundConfig handle move behind an always-allocated AudioSourceColdData blob, leaving only Source and ActiveEventID inline (the fields Scene::UpdateAudio's per-frame position-sync loop actually touches). ~150+ bytes -> 24 bytes. Both PrefabComponent and AudioSourceComponent are now classified non-trivial by OloHeaderTool's private-member auto-detection (their cold data lives behind a private std::unique_ptr), so they fall out of the generated AllComponents/SceneSerializer codegen automatically — no exclusion-set edits needed. PrefabComponent's scene YAML serializer is hand-written to keep the on-disk shape identical to the codegen'd version it replaces (same top-level keys, same sorted-sequence emit), so no scene migration is required. All call sites (SceneSerializer, SaveGame serializer, Scene/Prefab/SceneStreamer, C#/Lua script glue, editor panel, and tests) now go through the new accessor methods. static_assert(sizeof(...) <= 32) guards PrefabComponent and AudioSourceComponent against regressing back into hot-array bloat. Closes #444 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR moves ChangesComponent data-shape refactor and call-site migration
Sequence Diagram(s)sequenceDiagram
participant Scene as Scene::InitAudioRuntime
participant Component as AudioSourceComponent
participant AssetManager as AssetManager
participant Source as AudioSource
Scene->>Component: GetSoundConfigHandle()
alt preset handle present
Scene->>AssetManager: Load SoundConfigAsset
AssetManager-->>Scene: preset config
Scene->>Component: GetConfig() / apply preset
end
Scene->>Component: GetUseEventSystem() / GetStartCommandID()
alt event system enabled
Scene->>Scene: post trigger or start playback
else
Scene->>Source: set params from GetConfig()
Scene->>Source: play on awake
end
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- AudioSourceComponent: add move ctor/assignment so relocating it in the ECS pool no longer deep-copies the cold data behind the unique_ptr. - SceneSerializer: sanitize every AudioSourceConfig float on the scene-YAML load path (volume/pitch/rolloff/gain/distance/cone/doppler), mirroring the ranges SaveGameComponentSerializer already enforces, so a corrupted or hand-edited .olo file can't inject NaN/Inf into the audio engine. - SceneHierarchyPanel: extract the duplicated start-event CommandID resolution logic (checkbox toggle + InputText handler) into a single ResolveStartCommandID helper. Addresses CodeRabbit review on PR #578 (issue #444).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@OloEngine/src/OloEngine/Scene/SceneSerializer.cpp`:
- Around line 1735-1749: `SceneSerializer` is using mismatched editor and
serialization limits for audio cone angles and gain, causing save/load drift.
Update the `TrySetDsp` calls for `ConeInnerAngle` and `ConeOuterAngle` to use
radians consistently with `AudioSourceConfig`, and align `MaxGain`’s allowed
range with the editor so it matches the serializer’s clamp. Keep the
ranges/units consistent in the scene serialization path where `config` is
sanitized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc7e2fde-9134-4c66-a656-1c3c738f1ea8
📒 Files selected for processing (3)
OloEditor/src/Panels/SceneHierarchyPanel.cppOloEngine/src/OloEngine/Scene/Components.hOloEngine/src/OloEngine/Scene/SceneSerializer.cpp
|
The AudioListenerComponent deserialize path used raw TrySet with no sanitization, unlike the AudioSourceComponent path, so out-of-range or non-finite cone values loaded straight through. Clamp cone angles to [0, 2pi] radians and ConeOuterGain to [0, 1] on load, matching the AudioSource path and AudioListenerConfig defaults. Editor drift fixes (SceneHierarchyPanel): - AudioSource Max Gain slider allowed 0..2 while every serializer clamps to 0..1, so 1.x collapsed to 1.0 on reload; range now 0..1. - Audio Source/Listener cone sliders drove the radians-valued cone fields with a 0..360 degrees range, storing e.g. 60 as 60 radians (clamped to ~6.28 on reload). Now edited in degrees with glm::degrees/radians conversion so the 0..360 UI maps to the radians storage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Issue #444 — move editor-only / cold data out of hot runtime ECS components to shrink their per-instance memory footprint.
PrefabComponent— the threestd::unordered_set<std::string>override-tracking sets (overridden/added/removed) move behind a lazily-allocatedPrefabOverrideSets(std::unique_ptr, null when a prefab instance has no overrides — the common case).sizeof(PrefabComponent): ~184-208 bytes → 24 bytes.TagComponent— the transientrenamingeditor-only flag moves out toSceneHierarchyPanel::m_RenamingEntityUUID(panel-local state; only one entity can be mid-rename at a time, so it doesn't need to live on the runtime component).AudioSourceComponent— playback config, event wiring, and the SoundConfig asset handle move behind an always-allocatedAudioSourceColdDatablob, leaving onlySourceandActiveEventIDinline (the only fieldsScene::UpdateAudio's per-frame position-sync loop touches).sizeof(AudioSourceComponent): ~150+ bytes → 24 bytes.Both
PrefabComponentandAudioSourceComponentare now automatically classified non-trivial by OloHeaderTool's private-member detection (their cold data lives behind a privatestd::unique_ptr), so they fall out of the generatedAllComponents/SceneSerializercodegen paths with no exclusion-set edits needed.PrefabComponent's scene YAML serializer is hand-written to preserve the exact on-disk shape the codegen'd version had (same top-level keys, same sorted-sequence emit) — no scene migration required. All call sites (SceneSerializer,SaveGameserializer,Scene/Prefab/SceneStreamer, C#/Lua script glue, the editor panel, and tests) now go through the new accessor methods (GetConfig(),GetOverriddenComponents(),SetSoundConfigHandle(), etc.).static_assert(sizeof(...) <= 32)guardsPrefabComponentandAudioSourceComponentagainst regressing back into hot-array bloat.Closes #444
Test plan
GenerateBindingsrebuilt — all generated.inlfiles reported "(unchanged)", confirming the codegen classifier already excludes both split components via private-member auto-detection.OloEngine-Testssuite: 4182 passed, 5 skipped (no GL context / launch smoke — expected in this environment), 0 failed.ComponentTupleCoverageTest,ComponentSerializerCoverageTest,ComponentHandlerCoverageTest,SaveGameComponentSerializerCoverageTest,PrefabOverrideTest,ComponentRoundTrip.*,AudioEventsSceneIntegrationTest,LuaBindingTest.OloEditorbuilds clean.sizeof()verified via a temporary throwaway test (not committed):PrefabComponent=24,AudioSourceComponent=24 (both previously well over 150 bytes).TagComponentstays 40 in this Debug build because MSVC's debug-iteratorstd::stringoverhead already pads to a multiple of 8 regardless of the trailing bool; the real win shows up in Release builds wherestd::stringis 32 bytes (40→32)./code-review high— 8 independent finder angles (line-by-line, removed-behavior audit, cross-file call-site tracer, reuse, simplification, efficiency, altitude, CLAUDE.md conventions) found zero correctness bugs. A few low-severity duplication notes (repeated accessor triples, repeated YAML sort/emit blocks) were surfaced but not acted on — they mirror the project's explicit anti-over-engineering guidance ("three similar lines is better than a premature abstraction") and an existing pattern already present elsewhere inSceneSerializer.cpp.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes