MAP-18: line memory usage #696
Conversation
WalkthroughThis update simplifies how line metadata is handled across the iOS graphics and shared mapping modules. In the Swift and Metal shader files, several vertex attributes have been consolidated or removed, with new bitwise operations established to pack and unpack metadata. A helper method is added to centralize metadata packing. Client code in shared layer object files has been updated to call the new helper, streamlining the generation and use of line style information in the rendering pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant L as Layer Object
participant LH as LineHelper
participant S as LineShader
L->>LH: Call packLineStyleMetadata(vertexIndex, lineStyleIndex, segmentType, prefixTotalLineLength)
LH-->>L: Return packedMetadata
L->>S: Pass lineMetadata attribute (packedMetadata)
S->>S: Unpack metadata using bitwise operations
S->>S: Extract vertexIndex, styleOffset, and segmentType for rendering
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
stmitt
force-pushed
the
feature/MAP-18-line-memory-usage
branch
from
256d8cf to
1998e48
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
shared/public/LineHelper.h (1)
75-85: Well-implemented bit packing method for memory optimization.The
packLineStyleMetadatafunction efficiently packs multiple attributes into a single float value using bitwise operations:
- vertexIndex (2 bits) - supports values 0-3
- lineStyleIndex (8 bits) - supports values 0-255
- segmentType (2 bits) - supports values 0-3
- prefixTotalLineLength (21 bits) - scaled by 1000
This implementation aligns perfectly with the PR objective of reducing memory usage for line vertices.
However, consider adding documentation to clarify:
- The expected range of each parameter
- The potential precision loss for
prefixTotalLineLengthwhen multiplied by 1000 and truncated to 21 bitsinline static float packLineStyleMetadata(uint8_t vertexIndex, uint8_t lineStyleIndex, uint8_t segmentType, float prefixTotalLineLength) { + // vertexIndex: 0-3 (2 bits) + // lineStyleIndex: 0-255 (8 bits) + // segmentType: 0-3 (2 bits) where 0=inner, 1=start, 2=end, 3=both start and end + // prefixTotalLineLength: scaled by 1000 and stored in 21 bits (precision limited to ~2097 units) uint32_t packed = (vertexIndex & 0x3) | ((lineStyleIndex & 0xFF) << 2) | ((segmentType & 0x3) << 10) | ((int(prefixTotalLineLength * 1000.0) & 0x1FFFFF) << 11); float result; std::memcpy(&result, &packed, sizeof(float)); // Preserve all 32 bits return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ios/graphics/Model/Line/LineVertex.swift(2 hunks)ios/graphics/Shader/Metal/LineShader.metal(18 hunks)shared/public/LineHelper.h(1 hunks)shared/src/map/layers/objects/Line2dLayerObject.cpp(3 hunks)shared/src/map/layers/objects/LineGroup2dLayerObject.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build JVM Project / build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (25)
ios/graphics/Model/Line/LineVertex.swift (2)
33-34: Good optimization: Consolidated vertex attributes into a single metadata field.This change aligns with the PR objective of reducing memory usage for line vertices. By changing the attribute from "Vertex Index" to "Metadata", you're now packing multiple pieces of information into a single field, which will reduce memory consumption per vertex.
60-61: Consistent implementation of metadata consolidation.The same metadata consolidation approach is correctly applied to both descriptor implementations (unit sphere and regular), ensuring consistency across different rendering contexts.
shared/src/map/layers/objects/Line2dLayerObject.cpp (3)
13-13: Correctly added LineHelper.h dependency.Good addition of the LineHelper.h include to access the new packLineStyleMetadata function.
64-66: Improved segment type calculation with clear conditional logic.The new segmentType calculation is more readable and explicit than the previous bitwise approach. The values clearly indicate:
- 3: both start and end of line (single segment)
- 1: start of line
- 2: end of line
- 0: inner segment
This makes the code more maintainable and easier to understand.
82-82: Effective consolidation of metadata using the new helper method.Using
LineHelper::packLineStyleMetadatasuccessfully consolidates multiple attributes (vertex index, segment type, and line length prefix) into a single value, reducing memory usage in line with the PR objective.shared/src/map/layers/objects/LineGroup2dLayerObject.cpp (3)
15-15: Correctly added LineHelper.h dependency.Good addition of the LineHelper.h include to access the new packLineStyleMetadata function.
68-70: Consistent segment type calculation across layer objects.The segment type calculation matches the implementation in Line2dLayerObject.cpp, ensuring consistency between different layer objects. This is good for maintainability and reduces cognitive load when working with both classes.
86-86: Effective consolidation of metadata using the new helper method.Using
LineHelper::packLineStyleMetadatasuccessfully consolidates multiple attributes into a single value, passing in the lineStyleIndex which is appropriately different for each line in the group. This change reduces memory usage while maintaining all necessary functionality.ios/graphics/Shader/Metal/LineShader.metal (17)
29-35: Good introduction of unified metadata.
The new comment block clearly explains how multiple pieces of line-metadata are packed into a single float, which is a solid approach for reducing attribute usage.
41-47: Consistent metadata strategy across structs.
Duplicating the metadata packing comments ensures clarity for maintenance, and using the same packed format in both 2D and unit-sphere vertices is helpful for consistency.
55-55: Changed output fields align with new packed metadata.
Usingint styleOffsetanduint segmentTypecomplements the bit extraction logic. Confirm thatsegmentTypefits all valid segment states (0–3) to avoid unexpected values.Also applies to: 57-57
118-125: Bitwise packing logic validation.
Reinterpreting the float as a uint and then extracting bits (0–1 for vertex index, 2–9 for style index, and 10–11 for segment type) is correct. Multiplying the style index by 8 matches the size of the SimpleLineStyling struct (8 half fields). Ensure there are no out-of-bounds accesses for large style indices.
137-139: Vertex index branch logic looks consistent.
The four possible vertex indices (0–3) are handled. Since the code sets two bits for the vertex index, default cases are not needed. The transformations for each branch appear sound.Also applies to: 149-149, 152-152, 154-154, 156-156
169-169: Style offset usage.
Populating.styleOffset = styleOffsetensures the fragment shader can retrieve the correct styling data. This is consistent with prior lines.
187-193: Reused bitwise approach for simple lines.
Extracting the bits in the same way preserves consistency. Double-check thatstyleIndex * 8does not risk out-of-range indexing in the styling buffer.
206-208: Vertex index branch logic.
The conditions correctly flip length or width normals based on the vertex index for shaping the line strip. This matches the same pattern used above.Also applies to: 218-218, 221-221, 223-223, 225-225
238-238: Storing style offset in output struct.
ForwardingstyleOffsetensures the fragment shader has the correct pointer base for styling. No issues spotted.
252-252: Valid pointer cast for styling.
Casting the pointer from the offset is a low-level operation; verify the offset multiplied by “8” half fields is correct to avoid buffer overflows.
307-314: Extended bit packing includes length prefix.
The added shift(packed >> 11) & 0x1FFFFFfor 21 bits allows storing sizable path lengths. Dividing by 1000 is a straightforward scaling factor, but verify that the 21-bit range is large enough for use cases (max ~2097.151). Multiplying styleIndex by 23 is correct for the larger LineStyling struct.
332-334: Vertex index manipulation on the unit sphere.
The approach of flipping normals or leaving them “as is” based on the vertex index is consistent with 2D logic above; no immediate red flags here.Also applies to: 344-344, 347-347, 349-349, 351-351
366-366: Carrying additional metadata in the output struct.
Adding.styleOffset,.segmentType, and.lengthPrefixensures the fragment shader can access all style and length details. Looks good.Also applies to: 369-369, 371-371
387-394: Consistent bitwise logic for lineGroupVertexShader.
Extracting segmentType, styleIndex, and lengthPrefix mirrors the prior function. Check for potential out-of-range scenarios for styleIndex in advanced use cases.
412-414: Matching vertex index branches to 2D logic.
Same approach to flipping direction vectors for different corners of the line quad. Maintains parallel flow with the sphere version.Also applies to: 424-424, 427-427, 429-429, 431-431
446-446: Populating style, segment, and length fields on the output struct.
Ensures all relevant metadata flows to the fragment shader. The logic is consistent with earlier code.Also applies to: 449-449, 450-450
464-464: Pointer dereference in lineGroupFragmentShader.
Reusing.styleOffsetto compute the pointer ensures that style data is consistent with the geometry stage. Validate that the offset does not exceed styling buffer size.
Summary by CodeRabbit
New Features
Refactor
These updates improve the overall consistency and efficiency of line rendering, offering a smoother and more reliable visual experience.