Skip to content

Conversation

@finscn
Copy link
Contributor

@finscn finscn commented Dec 17, 2025

详见 #19104 中的讨论

Re: #

Changelog


Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.


Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

Greptile Summary

This PR refactors the vDataCountLimit calculation in MeshBuffer.initialize() to address issues identified in #19104:

Key Changes:

  • Fixed comment accuracy: The old comment said "2^16 - 1" but the value was correctly 65536. Since the check uses < (not <=), 65536 is correct to allow vertices 0-65535. The new comment correctly states "2^16"
  • Dynamic limit calculation: Changed from hardcoded values (65536 and 4294967295) to a formula based on macro.BATCHER2D_MEM_INCREMENT * 256, which represents the maximum float count that fits in the allocated memory
  • Simplified GPU feature detection: Replaced complex API type checks with a direct hasFeature(ELEMENT_INDEX_UINT) check, which is cleaner and more maintainable
  • Removed unused import: Dropped the API enum import since it's no longer needed

How it works:
The new formula vDataCountLimit = macro.BATCHER2D_MEM_INCREMENT * 256 calculates the total number of floats that fit in memory (since BATCHER2D_MEM_INCREMENT is in KB, and each float is 4 bytes: KB * 1024 / 4 = KB * 256). For devices without ELEMENT_INDEX_UINT support, this is capped at 65536 vertices (the Uint16 index limit). The assertion initVDataCount / floatsPerVertex < vDataCountLimit ensures the requested vertex count doesn't exceed either the memory-based limit or the GPU indexing limit.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes are well-reasoned improvements that fix documented issues from MeshBuffer 中的 bug, 以及 关于 65535 和 65536 的小建议 #19104. The refactoring makes the code more maintainable by deriving limits from BATCHER2D_MEM_INCREMENT instead of hardcoding magic numbers, fixes comment accuracy (2^16 vs 2^16-1), and simplifies GPU feature detection. The logic is mathematically sound and the check correctly prevents buffer overflow by ensuring vertex counts don't exceed either memory allocation or GPU indexing limits
  • No files require special attention

Important Files Changed

Filename Overview
cocos/2d/renderer/mesh-buffer.ts Improved vDataCountLimit calculation to derive from BATCHER2D_MEM_INCREMENT, fixed comment accuracy, removed unused API import, simplified GPU feature detection logic

Sequence Diagram

sequenceDiagram
    participant Caller as BufferAccessor
    participant MB as MeshBuffer
    participant Device as GPU Device
    participant Macro as macro.BATCHER2D_MEM_INCREMENT
    
    Caller->>Macro: Read BATCHER2D_MEM_INCREMENT (144 KB default)
    Caller->>Caller: Calculate vCount = floor(144 * 1024 / vertexFormatBytes)
    Caller->>Caller: Calculate initVDataCount = vCount * floatsPerVertex
    Caller->>MB: initialize(device, attrs, initVDataCount, iCount)
    
    MB->>MB: Calculate floatsPerVertex from attrs
    MB->>Macro: Read BATCHER2D_MEM_INCREMENT
    MB->>MB: vDataCountLimit = BATCHER2D_MEM_INCREMENT * 256
    MB->>Device: Check hasFeature(ELEMENT_INDEX_UINT)
    
    alt Device lacks ELEMENT_INDEX_UINT
        Device-->>MB: false (WebGL1 without extension)
        MB->>MB: vDataCountLimit = min(vDataCountLimit, 65536)
        Note over MB: Cap at 2^16 vertices (Uint16 indices 0-65535)
    else Device has ELEMENT_INDEX_UINT
        Device-->>MB: true (WebGL2/WebGPU/WebGL1+ext)
        Note over MB: Use full memory-based limit
    end
    
    MB->>MB: Assert: initVDataCount / floatsPerVertex < vDataCountLimit
    
    alt Assertion passes
        MB->>MB: Allocate vData and iData buffers
        MB-->>Caller: Initialization complete
    else Assertion fails
        MB->>MB: Throw error 9005
        Note over MB: BATCHER2D_MEM_INCREMENT too large<br/>for GPU capabilities
    end
Loading

@finscn finscn changed the title 更新计算 vDataCountLimit 的逻辑 更新计算 vDataCountLimit 的逻辑. Dec 17, 2025
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (3)

  1. cocos/2d/renderer/mesh-buffer.ts, line 254-259 (link)

    logic: Unit mismatch in comparison: vDataCountLimit is in floats but compared against vertex count

    vDataCountLimit = macro.BATCHER2D_MEM_INCREMENT * 256 calculates max float count (KB * 1024 / 4 bytes per float).

    But line 259 compares this._initVDataCount / this._floatsPerVertex (vertex count) against vDataCountLimit (float count).

    Fix by dividing by this._floatsPerVertex:

  2. cocos/2d/renderer/mesh-buffer.ts, line 259 (link)

    logic: Should use <= instead of < since exactly reaching the limit is valid (65536 vertices with indices 0-65535 fits in 16-bit indices)

  3. cocos/2d/renderer/mesh-buffer.ts, line 254 (link)

    style: Error message 9005 in EngineErrorMap.md:3190 assumes fixed 9 floats per vertex, but actual vertex format varies. Consider updating error message to reflect dynamic floatsPerVertex.

1 file reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

WEBGPU 和 WEBGL2 环境中   device.hasFeature(Feature.ELEMENT_INDEX_UINT) == true ,

所以只用这一个条件判断就可以
@github-actions
Copy link

Code Size Check Report

Wechat (WASM) Before After Diff
2D Empty (legacy pipeline) 1009738 bytes 1009762 bytes ⚠️ +24 bytes
2D All (legacy pipeline) 2675347 bytes 2675371 bytes ⚠️ +24 bytes
2D All (new pipeline) 2767083 bytes 2767107 bytes ⚠️ +24 bytes
(2D + 3D) All 10024081 bytes 10024105 bytes ⚠️ +24 bytes
Web (WASM + ASMJS) Before After Diff
(2D + 3D) All 16843143 bytes 16843167 bytes ⚠️ +24 bytes

Interface Check Report

This pull request does not change any public interfaces !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant