fix(legacy): sync instanced buffer batching optimization to native - #293
Conversation
|
@cocos-robot run test cases |
There was a problem hiding this comment.
Pull request overview
This PR syncs the instanced-buffer batching optimizations from the engine (TS) side to the native renderer implementation, aiming to reduce CPU overhead when merging instanced draws and align native behavior with the existing instancing path.
Changes:
- Add a key-based cache (
_instancesMap) in nativeInstancedBufferto avoid scanning all instance groups on every merge. - Refactor native merge logic into
_appendInstance()/_createInstance()helpers and expand merge compatibility inputs (lightmap + reflection probe bindings + stride). - Update TS instanced-buffer instance creation to persist the additional compatibility fields (and minor buffer-size refactor).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| native/cocos/renderer/pipeline/InstancedBuffer.h | Adds helper declarations and an instance-group lookup map keyed by merge-compatibility inputs. |
| native/cocos/renderer/pipeline/InstancedBuffer.cpp | Implements cached merge path, refactors append/create logic, and adjusts upload behavior (needs follow-up per review). |
| cocos/rendering/instanced-buffer.ts | Passes/stores additional merge-compatibility fields in created instance items and minor allocation refactor. |
Suppressed comments (1)
native/cocos/renderer/pipeline/InstancedBuffer.h:89
Uint8Array bufferis passed by value to_createInstance(), which copies the underlyingArrayBuffer::Ptr. Preferconst Uint8Array&to avoid per-merge refcount traffic (and update the .cpp definition accordingly).
void _createInstance(const ccstd::string &key,
gfx::InputAssembler *sourceIA,
const ccstd::vector<gfx::Attribute> &attributes,
Uint8Array buffer,
uint32_t stride,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Size Check Report
Interface Check ReportThis pull request does not change any public interfaces ! |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
native/cocos/renderer/pipeline/InstancedBuffer.cpp:143
CC_ASSERT(destination)is ineffective here becausedestinationis computed via pointer arithmetic oninstance.data. Ifinstance.datais null (e.g. allocation failure), the pointer arithmetic is already undefined behavior before the assert. Assert the base pointer (or handle allocation failure) before computingdestination.
auto *destination = instance.data + static_cast<size_t>(instance.stride) * instance.drawInfo.instanceCount;
CC_ASSERT(destination);
memcpy(destination, buffer.buffer()->getData(), instance.stride);
native/cocos/renderer/pipeline/InstancedBuffer.cpp:213
- The comment says the upload size is "clamped", but the code only asserts size relationships (and
CC_ASSERTcompiles out in release). AlsodataCapacityis only used inCC_ASSERT, so it becomes an unused variable in non-debug builds. Consider removing the unused variable and making the comment match the actual behavior (uploading only the valid instance data range).
// `instance.data` is only guaranteed to hold `instance.capacity * instance.stride` bytes.
// If `instance.vb` was resized to a larger size than the CPU-side `instance.data`
// buffer (e.g. due to a bug in the resize/append logic), copying `instance.vb->getSize()`
// bytes from `instance.data` would read out of bounds and crash. Clamp to the smaller
// of the two to avoid reading past the end of `instance.data`.
|
@troublemaker52025, Please check the result of
Task Details |
|
@troublemaker52025, Please check the result of
Task Details |
|
@troublemaker52025, Please check the result of
Task Details
|
|
@troublemaker52025, Please check the result of
Task Details |
|
@troublemaker52025, Please check the result of
Task Details |
|
@troublemaker52025, Please check the result of
Task Details |
|
@troublemaker52025, Please check the result of
Task Details
|
|
@troublemaker52025, Please check the result of
Task Details |
Re: #
Changelog
Continuous Integration
This pull request:
Compatibility Check
This pull request: