netcode performance: Add optional FP16 quantization; reduce replication overhead - #1343
netcode performance: Add optional FP16 quantization; reduce replication overhead#1343mcdubhghlas wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughScene replication adds per-property Full/Half precision settings. Quantized encoding and decoding support selected numeric Variants across spawn, delta, and synchronization packets. The editor, documentation, tests, and benchmarks expose and validate the new behavior. ChangesReduced-Precision Scene Replication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SceneReplicationConfig
participant SceneReplicationInterface
participant MultiplayerSynchronizer
participant ReplicationPeer
SceneReplicationConfig->>SceneReplicationInterface: provide channel precision vectors
SceneReplicationInterface->>MultiplayerSynchronizer: encode state with precisions
MultiplayerSynchronizer->>ReplicationPeer: write quantized or compressed payload
ReplicationPeer->>MultiplayerSynchronizer: provide received payload
MultiplayerSynchronizer->>SceneReplicationInterface: decode state with precisions
SceneReplicationInterface->>ReplicationPeer: apply replication state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
modules/multiplayer/scene_replication_config.cpp (1)
250-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
property_get_precisiona const method.
property_get_precisiononly readsproperties. It does not call_update(). The sibling gettersproperty_get_spawnandproperty_get_replication_modeareconst, and their documentation entries usequalifiers="const". The new getter breaks that convention, andSceneReplicationConfig.xmldeclaresproperty_get_precisionwithoutqualifiers="const"as a result. Addconstin the header and the definition, then addqualifiers="const"to the doc entry.♻️ Proposed change
-SceneReplicationConfig::ReplicationPrecision SceneReplicationConfig::property_get_precision(const NodePath &p_path) { +SceneReplicationConfig::ReplicationPrecision SceneReplicationConfig::property_get_precision(const NodePath &p_path) const { List<ReplicationProperty>::Element *E = properties.find(p_path); ERR_FAIL_COND_V(!E, PRECISION_FULL); return E->get().precision; }Note:
properties.find()on a constListreturns aconst Element *, so adjust the local type accordingly.🤖 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 `@modules/multiplayer/scene_replication_config.cpp` around lines 250 - 254, Make SceneReplicationConfig::property_get_precision const in both its declaration and definition, and use a const List<ReplicationProperty>::Element pointer for the properties.find result. Update the corresponding SceneReplicationConfig.xml documentation entry to include qualifiers="const", matching the sibling getter conventions.modules/multiplayer/tests/test_scene_replication.h (1)
107-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for values outside the half-float range.
The current cases use magnitudes that fp16 represents exactly.
SceneReplicationConfig.xmlstates thatPRECISION_HALFreduces range, but no test pins that behavior.
Math::make_half_floatmaps any magnitude above 65504 to infinity, and it flushes very small magnitudes to zero. A game that replicates a position on a large map therefore receivesinfrather than a clamped value. Add a case that encodes such a value and asserts the observed result. The test then documents the boundary and catches a future change in the conversion helper.💚 Proposed test
+TEST_CASE("[Multiplayer][SceneReplication] Half precision saturates outside the fp16 range") { + Vector<Variant> values; + values.push_back(Vector3(70000.0, 1.0, 0.0000001)); + Vector<int> precisions = { SceneReplicationConfig::PRECISION_HALF }; + + Vector<Variant> out = _round_trip(values, precisions); + const Vector3 v = out[0]; + CHECK(Math::is_inf(v.x)); + CHECK(v.y == doctest::Approx(1.0)); + CHECK(v.z == doctest::Approx(0.0)); +}🤖 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 `@modules/multiplayer/tests/test_scene_replication.h` around lines 107 - 129, Add an out-of-range half-precision case to the “[Multiplayer][SceneReplication] Half-precision codec round-trips supported types” test, covering a magnitude above 65504 and asserting that the round-trip result is infinity as produced by Math::make_half_float. Also cover a sufficiently small magnitude if needed to document its flush-to-zero behavior, while preserving the existing supported-type assertions.modules/multiplayer/tests/test_scene_replication_benchmark.h (1)
113-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSize the encode buffer from the sizing pass.
bufis fixed at 128 bytes, and line 121 and line 127 encode into it without a sizing pass.encode_state_quantizednever bounds-checksp_buffer; it writesr_lenbytes and reports the total throughr_len.The current
stateencodes to about 68 bytes at full precision, so the buffer holds today. The margin is invisible at the call site. Any future field added tostateat lines 63-68 overflows the heap buffer with no diagnostic.Call the sizing pass first and resize, as the
_encodehelper at lines 48-60 already does. The same pattern applies at line 293 and line 319, wherebufis sized by the formulaprop_count * 8 + 16.🛡️ Proposed change
Vector<uint8_t> buf; - buf.resize(128); + { + int max_size = 0; + MultiplayerSynchronizer::encode_state_quantized(ptrs.ptrw(), full.ptr(), ptrs.size(), nullptr, max_size, false); + buf.resize(max_size); + }🤖 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 `@modules/multiplayer/tests/test_scene_replication_benchmark.h` around lines 113 - 121, Replace the fixed 128-byte initialization of buf in the benchmark encoding loop with the existing sizing-pass pattern used by the _encode helper: call encode_state_quantized first to obtain the required length, resize buf accordingly, then perform the actual encode for each precision mode. Apply the same sizing-based allocation to the analogous buf usage around the later encode calls instead of relying on prop_count * 8 + 16.modules/multiplayer/multiplayer_synchronizer.cpp (1)
301-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or drop the
0x3Ftype mask, and consider validating the count.Two points on
decode_state_quantized:
Line 307 masks the type byte with
0x3F.encode_state_quantizedwrites the rawVariant::Typebyte with no flag bits, so the mask is not required here. The mask mirrors the compressed-variant header layout inMultiplayerAPI. Add a short comment that states why the mask exists, or remove it. Without a comment, a later reader can assume the quantized header carries flags.The function derives the element count from
r_variants.size()but indexesp_precisions[i]over the same range. The array length is not passed. Callers must guarantee thatp_precisionsholds at leastr_variants.size()entries. The delta receive path builds these two arrays from separate sources, so the guarantee is not local. See the related comment onmodules/multiplayer/scene_replication_interface.cpp.🤖 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 `@modules/multiplayer/multiplayer_synchronizer.cpp` around lines 301 - 327, Update decode_state_quantized around the type extraction and iteration bounds: remove the unnecessary 0x3F mask or add a concise comment documenting the quantized header’s flag layout, and validate that p_precisions contains at least r_variants.size() entries before indexing it. Preserve the existing decoding behavior while making the count guarantee explicit, coordinating with the delta receive path if needed.
🤖 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 `@modules/multiplayer/scene_replication_interface.cpp`:
- Around line 731-743: Bound the iteration in _delta_precisions to at most 64
watched properties before evaluating 1ULL << i, while preserving the existing
precision collection and reduced result behavior. Apply the same 64-property
bound in MultiplayerSynchronizer::get_delta_properties so both functions produce
matching property counts for decode_state_quantized.
- Around line 952-962: Update on_sync_receive after the decode_state_quantized
or decode_and_decompress_variants call to verify that decoding succeeded and
consumed exactly size bytes, matching the validation used by on_delta_receive.
Reject the payload and report the decode error when consumed differs from size
before applying the decoded vars through set_state.
---
Nitpick comments:
In `@modules/multiplayer/multiplayer_synchronizer.cpp`:
- Around line 301-327: Update decode_state_quantized around the type extraction
and iteration bounds: remove the unnecessary 0x3F mask or add a concise comment
documenting the quantized header’s flag layout, and validate that p_precisions
contains at least r_variants.size() entries before indexing it. Preserve the
existing decoding behavior while making the count guarantee explicit,
coordinating with the delta receive path if needed.
In `@modules/multiplayer/scene_replication_config.cpp`:
- Around line 250-254: Make SceneReplicationConfig::property_get_precision const
in both its declaration and definition, and use a const
List<ReplicationProperty>::Element pointer for the properties.find result.
Update the corresponding SceneReplicationConfig.xml documentation entry to
include qualifiers="const", matching the sibling getter conventions.
In `@modules/multiplayer/tests/test_scene_replication_benchmark.h`:
- Around line 113-121: Replace the fixed 128-byte initialization of buf in the
benchmark encoding loop with the existing sizing-pass pattern used by the
_encode helper: call encode_state_quantized first to obtain the required length,
resize buf accordingly, then perform the actual encode for each precision mode.
Apply the same sizing-based allocation to the analogous buf usage around the
later encode calls instead of relying on prop_count * 8 + 16.
In `@modules/multiplayer/tests/test_scene_replication.h`:
- Around line 107-129: Add an out-of-range half-precision case to the
“[Multiplayer][SceneReplication] Half-precision codec round-trips supported
types” test, covering a magnitude above 65504 and asserting that the round-trip
result is infinity as produced by Math::make_half_float. Also cover a
sufficiently small magnitude if needed to document its flush-to-zero behavior,
while preserving the existing supported-type assertions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23ff3489-79ac-4343-bdbc-a35f29eda126
📒 Files selected for processing (10)
modules/multiplayer/doc_classes/SceneReplicationConfig.xmlmodules/multiplayer/editor/replication_editor.cppmodules/multiplayer/editor/replication_editor.hmodules/multiplayer/multiplayer_synchronizer.cppmodules/multiplayer/multiplayer_synchronizer.hmodules/multiplayer/scene_replication_config.cppmodules/multiplayer/scene_replication_config.hmodules/multiplayer/scene_replication_interface.cppmodules/multiplayer/tests/test_scene_replication.hmodules/multiplayer/tests/test_scene_replication_benchmark.h
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 `@modules/multiplayer/tests/test_scene_replication.h`:
- Around line 169-170: Update the overflow assertions in the scene replication
test to require v.x to be positive infinity and v.y to be negative infinity,
using exact signed-infinity checks instead of accepting arbitrary infinity or
NaN values. Preserve the existing codec round-trip setup and assert the
documented signed overflow results for both components.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c26ea4f5-1fd7-4b5e-a22d-7c2967c33d61
📒 Files selected for processing (3)
modules/multiplayer/scene_replication_interface.cppmodules/multiplayer/tests/test_scene_replication.hmodules/multiplayer/tests/test_scene_replication_benchmark.h
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/multiplayer/tests/test_scene_replication_benchmark.h
- modules/multiplayer/scene_replication_interface.cpp
3 participants
benchmarks
Running the attached gdscript benchmark:
TL;DR on benchmarks:
PRECISION_HALFroughly halves replication bandwidth, is ~1.6 times faster {en,de}code, with small but magnitude-dependent precision loss. Cached precision lookup cuts reduced-precision encoding cost by 78–96%, while reference-based property gathering improves full sync ticks by ~12–17% for all replication modes.Bandwidth
Native: 56 -> 23 bytes.
GDScript: 56.2 -> 29.1 bytes
Encoding Speed
Full: 109ns
Half: 66.2ns
Cached Precision Lookup
8 props: 78% faster
32 props: 93% faster
64 proprs: 96% faster
GDScript Accuracy
POS: 0.037 units
ROT: 0.105 degrees
SCALE: 0.0012 units
O-Notation Changes
Changing copy to ref resulted in:
Property-list gather: O(n) -> O(1)
searching the property list for every property vs cached lookup resulted in
Precision lookup and encoding: O(n^2) -> O(n)
How to use quantization
Quantization is accessible via the setting
SceneReplicationConfig.PRECISION_HALFFor example:
Attached is a GDScript benchmark of FULL vs HALF. Be sure to change "USE_HALF" as needed for testing.
bench.zip
Summary by CodeRabbit