fix(web): 3d tiles' style cannot be reset #1262
Conversation
WalkthroughThe changes in this pull request enhance the styling capabilities for 3D tiles within the layer style definitions. Key modifications include the addition of a new empty object for the "3dtiles" property in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. |
![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
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts (2)
Line range hint
146-152: Consider adding JSDoc comments to document the 3D tiles style properties.While the implementation is correct, adding documentation would help other developers understand the purpose and impact of each property (color, colorBlendMode, pbr).
+/** + * Default style for 3D tiles visualization + * @property color - The base color for 3D tiles + * @property colorBlendMode - Determines how colors are blended ("highlight" shows original textures) + * @property pbr - Enables/disables physically-based rendering + */ export const threeDTilesStyle = { "3dtiles": { color: "#FFFFFF", colorBlendMode: "highlight", pbr: false } };
Line range hint
154-177: Consider internationalizing the height property name.The style uses a Japanese property name
計測高さin conditions. Consider using English property names or implementing a localization system for consistent maintenance.conditions: [ - ["${計測高さ}>=180", "color('#F9FD4C')"], - ["${計測高さ}>=120", "color('#F6CD3D')"], - ["${計測高さ}>=60", "color('#EBD384')"], - ["${計測高さ}>=31", "color('#9E79BA')"], - ["${計測高さ}>=12", "color('#5230C2')"], + ["${height}>=180", "color('#F9FD4C')"], + ["${height}>=120", "color('#F6CD3D')"], + ["${height}>=60", "color('#EBD384')"], + ["${height}>=31", "color('#9E79BA')"], + ["${height}>=12", "color('#5230C2')"], ["true", "color('#362C52')"] ]Good performance consideration with disabled shadows.
The
shadows: "disabled"setting is appropriate for buildings colored by height, as it can improve rendering performance when dealing with multiple 3D tiles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts(1 hunks)
🔇 Additional comments (1)
web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts (1)
21-22: LGTM! The empty 3dtiles object addition fixes the style reset issue.
Adding a default empty object for 3dtiles is the correct approach to handle style resets, as it prevents the undefined property issue mentioned in the PR objectives.
Let's verify that this is the only place where default styles are defined:
✅ Verification successful
The empty 3dtiles object addition in defaultStyle is correct and sufficient
After reviewing the codebase, I can confirm that:
- The
defaultStyleinpresetLayerStyles.tsis the only default style definition that needs the empty "3dtiles" object - Other style presets like
threeDTilesStyleandcolorBuildingsByHeightdefine specific 3dtiles styles but don't need to handle the reset case - The types in
types.tsconfirm that "3dtiles" is a validAppearanceType
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential default style definitions that might need the same fix
ast-grep --pattern 'export const $name = {
$$$
"3dtiles": $$$
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for style definitions and 3dtiles
rg -l "3dtiles" --type ts
# Then let's look for any default style or preset definitions
rg "export const.*Style.*=.*{" --type ts -A 5
Length of output: 788
Script:
#!/bin/bash
# Fix the regex and search for style definitions
rg "export const.*Style.*=" --type ts -A 5
# Also check the specific files we found that mention 3dtiles
cat "web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts"
cat "web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/types.ts"
Length of output: 55541
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
- Coverage 18.25% 18.25% -0.01%
==========================================
Files 623 623
Lines 67338 67339 +1
Branches 622 622
==========================================
Hits 12295 12295
- Misses 55043 55044 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Overview
After you assign a style to 3dtiles and reset, the style can't be reset properly on map. That's due to the '3dtiles' property is
undefined. We should give a default empty object{}as the default 3dtiles style.What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit