refactor(web, server): Refacting depth test against terrain to globe #1161
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ZTongci
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1161 +/- ##
==========================================
+ Coverage 15.18% 20.56% +5.37%
==========================================
Files 642 985 +343
Lines 63207 101829 +38622
Branches 792 660 -132
==========================================
+ Hits 9600 20937 +11337
- Misses 53607 79650 +26043
- Partials 0 1242 +1242
Flags with carried forward coverage won't be shown. Click here to find out more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/pkg/builtin/manifest_ja.yml (1)
92-97: Summary of changes and recommendation for system-wide testingThe changes in this file are focused on introducing a new global depth testing configuration (
globeDepthTestAgainstTerrain) to replace the previous terrain-specific configuration. This aligns well with the PR objectives of refactoring depth testing to work with a globe.While the implementation looks correct, I recommend:
- Conducting thorough system-wide testing to ensure all components that rely on depth testing are compatible with this new global configuration.
- Updating any relevant documentation or comments in other parts of the codebase that may reference the old terrain-specific depth testing.
- Considering adding a brief comment in the YAML file explaining the purpose and impact of this new global configuration for future maintainers.
server/pkg/builtin/manifest.yml (2)
197-204: New group for Globe DepthTest looks good, minor suggestion for clarity.The addition of the "Globe DepthTest Against Ground and Terrain" group is well-structured and follows the existing pattern in the file. The boolean field type is appropriate for this feature.
Consider slightly rewording the description for better clarity:
- description: Hides objects under the terrain. Depending on the loading status of the terrain, objects may be shown or hidden. + description: Hides objects under the terrain. Note: Object visibility may fluctuate based on terrain loading status.
Line range hint
359-367: New tile zoom level controls added, consider adding descriptions.The addition of
tile_minLevelandtile_maxLevelfields enhances control over tile visibility. These are well-placed within the "Tiles" group and use the appropriate 'number' type.Consider adding descriptions to these new fields for better clarity:
- id: tile_minLevel type: number title: Minimum zoom level min: 0 max: 30 + description: The minimum zoom level at which this tile layer becomes visible. - id: tile_maxLevel type: number title: Maximum zoom level min: 0 max: 30 + description: The maximum zoom level at which this tile layer remains visible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- server/pkg/builtin/manifest.yml (1 hunks)
- server/pkg/builtin/manifest_ja.yml (1 hunks)
- web/src/beta/utils/convert-object.ts (1 hunks)
🔇 Additional comments (3)
web/src/beta/utils/convert-object.ts (1)
80-81: LGTM! Verify impact on dependent code.The change from "terrain.depthTestAgainstTerrain" to "globeDepthTestAgainstTerrain.depthTestAgainstTerrain" aligns well with the PR objective of refactoring the depth test against terrain to globe. This new key structure provides a more specific categorization of the depth test property.
To ensure this change doesn't break any existing functionality, please run the following script to check for any occurrences of the old key in the codebase:
If the first command returns any results, those occurrences may need to be updated to use the new key.
✅ Verification successful
Verification Successful: The old key "terrain.depthTestAgainstTerrain" is no longer present, and the new key "globeDepthTestAgainstTerrain.depthTestAgainstTerrain" is correctly used in
web/src/beta/utils/convert-object.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the old key "terrain.depthTestAgainstTerrain" # Test: Search for the old key. Expect: No results, as it should have been updated. rg --type typescript --type javascript "terrain\.depthTestAgainstTerrain" # Test: Search for the new key. Expect: At least one result (this file). rg --type typescript --type javascript "globeDepthTestAgainstTerrain\.depthTestAgainstTerrain"Length of output: 242
Script:
#!/bin/bash # Description: Search for occurrences of the old key "terrain.depthTestAgainstTerrain" and the new key "globeDepthTestAgainstTerrain.depthTestAgainstTerrain" # Test: Search for the old key. Expect: No results, as it should have been updated. rg "terrain\.depthTestAgainstTerrain" --type-add 'ts=*.ts' --type-add 'js=*.js' -t ts -t js # Test: Search for the new key. Expect: At least one result (this file). rg "globeDepthTestAgainstTerrain\.depthTestAgainstTerrain" --type-add 'ts=*.ts' --type-add 'js=*.js' -t ts -t jsLength of output: 328
Script:
#!/bin/bash # Description: Search for occurrences of the old key "terrain.depthTestAgainstTerrain" and the new key "globeDepthTestAgainstTerrain.depthTestAgainstTerrain" # Test: Search for the old key. Expect: No results, as it should have been updated. rg "terrain\.depthTestAgainstTerrain" --glob "*.ts" --glob "*.js" # Test: Search for the new key. Expect: At least one result (this file). rg "globeDepthTestAgainstTerrain\.depthTestAgainstTerrain" --glob "*.ts" --glob "*.js"Length of output: 251
server/pkg/builtin/manifest_ja.yml (1)
92-97: Consider the impact of globalizing depth testing configurationThe move from a terrain-specific depth testing configuration to a global one (
globeDepthTestAgainstTerrain) is a significant change. While it aligns with the PR objectives, it may have broader implications on how other parts of the system interact with depth testing.To ensure this change doesn't negatively impact existing functionality, please consider the following:
- Verify that all parts of the system that previously relied on the terrain-specific depth testing now correctly use the new global configuration.
- Test various scenarios where depth testing is crucial to ensure the new global configuration provides the expected behavior across different use cases.
Could you provide some insights into how this change has been tested across the system? Additionally, it might be helpful to update the relevant documentation to reflect this architectural change.
server/pkg/builtin/manifest.yml (1)
Line range hint
1-1161: Changes align well with PR objectives, good job!The modifications in this file successfully implement the depth test functionality for the globe, as outlined in the PR objectives. The new "Globe DepthTest Against Ground and Terrain" group and the additional tile zoom level controls are well-structured and consistent with the existing file format.
These changes effectively support the refactoring of the depth test functionality to work against a globe instead of just terrain, as mentioned in the PR summary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/pkg/builtin/manifest.yml (2)
197-203: LGTM! Consider enhancing the description.The addition of the "Globe DepthTest Against Ground and Terrain" group is well-implemented and aligns with the PR objectives. It provides a clear way to control the visibility of objects beneath the terrain.
Consider adding a more descriptive explanation for the "depthTestAgainstTerrain" field. For example:
- id: depthTestAgainstTerrain type: bool title: Enable + description: When enabled, objects beneath the terrain will be hidden based on the globe's curvature and terrain elevation.This would provide users with a clearer understanding of the feature's functionality.
Line range hint
332-340: LGTM! Consider adding descriptions for new fields.The addition of "tile_minLevel" and "tile_maxLevel" fields in the "Tiles" group is a good enhancement, providing more granular control over tile visibility at different zoom levels.
Consider adding descriptions for these new fields to improve clarity for users:
- id: tile_minLevel type: number title: Minimum zoom level min: 0 max: 30 + description: The minimum zoom level at which tiles will be displayed. Lower values show tiles from further away. - id: tile_maxLevel type: number title: Maximum zoom level min: 0 max: 30 + description: The maximum zoom level at which tiles will be displayed. Higher values allow for more detailed tiles when zoomed in close.These descriptions would help users understand the impact of adjusting these values.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- server/pkg/builtin/manifest.yml (1 hunks)
- server/pkg/builtin/manifest_ja.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pkg/builtin/manifest_ja.yml
🔇 Additional comments (1)
server/pkg/builtin/manifest.yml (1)
Line range hint
1-1037: Overall, the changes look good and align with the PR objectives.The modifications to the manifest file successfully implement the depth test refactoring for the globe and enhance tile control. The new "Globe DepthTest Against Ground and Terrain" group and the additional tile zoom level controls are well-integrated into the existing structure.
Key points:
- The depth test functionality now operates against a globe, as intended.
- Tile visibility can be more finely controlled with the new minimum and maximum zoom level fields.
The code maintains good consistency with the existing manifest format, and no major issues were identified. The minor suggestions for improved descriptions will enhance user understanding of these new features.
Overview
What I've done
Refacting depth test against terrain to globe
What I haven't done
How I tested
Check the UI on FE
Add a sketch layer, add a marker, style with point
Enable/disable and check the map, half of the point should be invisible when this option been enabled.
Refresh page and check whether the value is well saved.
Test on published page.
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Documentation