feat(engine): enhance texture resolution handling and add distance per pixel calculations #572
Conversation
…r pixel calculations
WalkthroughThe changes in this pull request involve significant updates to the texture handling and tile processing within the engine. Key modifications include the replacement of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
engine/runtime/common/src/texture.rs (2)
85-85: Consider consistent numeric types to avoid unnecessary castingAt line 85, you're casting
downsample_scaletof64and then the result back tof32. This may introduce minor precision issues.If precision permits, perform calculations using
f32throughout the function:-(error_factor * downsample_scale as f64).clamp(0.0, 1.0) as f32 +(error_factor as f32 * downsample_scale).clamp(0.0, 1.0)This change eliminates the need for casting and keeps the calculations consistent.
51-55: Update function documentation to reflect parameter changesThe function
get_texture_downsample_scale_of_polygonno longer acceptslimit_texture_resolutionas an optional parameter. The documentation should be updated to match the current function signature.Revise the documentation to accurately describe the parameters and purpose of the function.
engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (1)
480-487: Simplify thedownsample_scalecalculation for clarity.The conditional assignment of
downsample_scalecan be streamlined to enhance readability.Apply this diff to simplify the logic:
let downsample_scale = if limit_texture_resolution.unwrap_or(false) { get_texture_downsample_scale_of_polygon( &original_vertices, texture_size, ) as f32 } else { 1.0 }; +let downsample_scale = downsample_scale.max(1.0);Alternatively, ensure that
get_texture_downsample_scale_of_polygonreturns1.0whenlimit_texture_resolutionisfalse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (8 hunks)
- engine/runtime/action-sink/src/file/cesium3dtiles/slice.rs (1 hunks)
- engine/runtime/common/src/texture.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (3)
315-315: Verify the impact of increasingtexture_cachesize on memory usage.The
texture_cachesize has been increased from100_000_000to200_000_000. While this may improve performance by caching more textures, it could also lead to higher memory consumption. Ensure that the system can handle this change without adverse effects on memory resources.
522-524: Ensure texture dimensions are within supported limits.While adjusting
max_widthandmax_heightto the next power of two is beneficial, confirm that the resulting dimensions do not exceed hardware texture size limitations to maintain compatibility across different devices.
541-541: Confirm removal ofJpegAtlasExporterreferences.Since
WebpAtlasExporteris now used, ensure all instances ofJpegAtlasExporterhave been removed from the codebase to prevent any deprecated code from lingering.Run the following script to identify any remaining references:
#!/bin/bash # Description: Check for residual uses of `JpegAtlasExporter`. # Test: Search the codebase for `JpegAtlasExporter`. Expect: No matches. rg --type rust 'JpegAtlasExporter'
…_distance_per_pixel
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
engine/runtime/common/src/texture.rs (1)
1-5: Approve constant change with minor comment suggestion.The change from
MAX_TEXTURE_PIXELS_PER_METERtoMIN_METER_PER_PIXELis a good improvement, allowing for higher resolution textures while maintaining a clear limit. The comment explains the reasoning well.Consider adding the equivalent pixels per meter value to the comment for clarity:
/// The pixel resolution should be limited to around 10cm (0.1m), /// but this means that signs and other objects will not be visible, so adjustments are necessary. +/// This value (0.025m/pixel) is equivalent to 40 pixels per meter. const MIN_METER_PER_PIXEL: f64 = 0.025;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- engine/runtime/common/src/texture.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
engine/runtime/common/src/texture.rs (1)
51-66: Approve simplification ofget_texture_downsample_scale_of_polygon.The changes to this function are well-implemented:
- Removal of the
limit_texture_resolutionparameter simplifies the function signature.- Utilization of the new
get_distance_per_pixelfunction improves code modularity.- The simplified logic using
MIN_METER_PER_PIXELis more straightforward and consistently applies the resolution limit.These modifications enhance the function's clarity and maintainability.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Improvements
Bug Fixes