【wip】ポリゴンの頂点をタイル境界にスナップする #678
Conversation
Walkthroughこのプルリクエストでは、 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
demo/cesium/3dtiles.html (1)
75-75: デバッグ設定の環境分けを推奨します
enableDebugWireframeオプションがハードコードされていますが、開発環境とプロダクション環境で設定を分けることを推奨します。以下のような環境変数による制御を検討してください：
- { enableDebugWireframe: true } + { enableDebugWireframe: process.env.NODE_ENV === 'development' }nusamai/src/sink/cesiumtiles/slice.rs (2)
18-23: 構造体のドキュメントとデライブマクロの追加を推奨タイル境界を表す構造体として適切ですが、以下の改善を推奨します：
- ドキュメントコメントの追加
DebugとCloneデライブマクロの追加+/// タイルの境界を表す構造体 +/// +/// # フィールド +/// * `min_x` - X座標の最小値 +/// * `max_x` - X座標の最大値 +/// * `min_y` - Y座標の最小値 +/// * `max_y` - Y座標の最大値 +#[derive(Debug, Clone)] struct TileBounds { min_x: f64, max_x: f64, min_y: f64, max_y: f64, }
406-411: 境界値の定数化とドキュメント化を推奨ハードコードされた境界値（0.0から1.0）は正規化されたタイル座標を表していると思われますが、これを明確にするために以下の改善を推奨します：
+/// 正規化されたタイル座標の境界値 +const NORMALIZED_TILE_MIN: f64 = 0.0; +const NORMALIZED_TILE_MAX: f64 = 1.0; + let tile_bounds = TileBounds { - min_x: 0.0, - max_x: 1.0, - min_y: 0.0, - max_y: 1.0, + min_x: NORMALIZED_TILE_MIN, + max_x: NORMALIZED_TILE_MAX, + min_y: NORMALIZED_TILE_MIN, + max_y: NORMALIZED_TILE_MAX, };
demo/cesium/3dtiles.html
Outdated
| "/examples/3dtiles_textured", | ||
| { enableDebugWireframe: true } |
There was a problem hiding this comment.
💡 Codebase verification
タイルセットのパスが無効です - 修正が必要
検証の結果、以下の問題が確認されました：
/examples/3dtiles_texturedディレクトリが存在しません- 現在利用可能な3DTilesのサンプルは以下のみです：
/examples/ext_mesh_features/examples/ext_structural_metadata
タイルセットのパスを既存のディレクトリのいずれかに変更するか、新しいタイルセットを適切に配置する必要があります。
🔗 Analysis chain
3Dタイルセットの読み込み設定の変更について確認が必要です
タイルセットのURLが変更され、デバッグオプションが追加されました。以下の点について確認をお願いします：
- 新しいパス
/examples/3dtiles_texturedが正しく設定されているか - テクスチャ付きタイルセットへの移行による影響
- デバッグワイヤーフレーム表示がタイル境界へのスナップ機能の開発に有用か
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 新しいタイルセットのパスが存在するか確認
fd -t d "3dtiles_textured$" examples/
# タイルセット関連の設定ファイルを検索
fd -e json "tileset.json$" examples/3dtiles_textured
Length of output: 304
Script:
#!/bin/bash
# リポジトリ内の全ての3DTilesに関連するディレクトリを検索
fd -t d "3dtiles"
# リポジトリ内のtileset.jsonファイルを検索
fd -e json "tileset.json$"
# HTMLファイル内のタイルセットパスの参照を検索
rg -g "*.html" "3dtiles_textured"
# デモディレクトリの構造を確認
ls -la demo/cesium/
Length of output: 788
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
nusamai/src/sink/cesiumtiles/slice.rs (2)
18-23: 構造体のドキュメントとデバッグサポートの追加を推奨構造体の目的と各フィールドの説明を追加することで、コードの理解性が向上します。また、デバッグ時の利便性のために、一般的な派生トレイトを追加することを推奨します。
+/// タイルの境界を表す構造体 +/// +/// # フィールド +/// * `min_x` - X座標の最小値 +/// * `max_x` - X座標の最大値 +/// * `min_y` - Y座標の最小値 +/// * `max_y` - Y座標の最大値 +#[derive(Debug, Clone, PartialEq)] struct TileBounds { min_x: f64, max_x: f64, min_y: f64, max_y: f64, }
406-427: タイル境界の固定値の意図を明確にすることを推奨タイル境界に0.0から1.0の固定値を使用している理由をコメントで説明することで、将来のメンテナンス性が向上します。また、これらの値を定数として定義することも検討してください。
+ // タイル空間は0.0から1.0の正規化された座標系を使用 + const TILE_MIN: f64 = 0.0; + const TILE_MAX: f64 = 1.0; + let tile_bounds = TileBounds { - min_x: 0.0, - max_x: 1.0, - min_y: 0.0, - max_y: 1.0, + min_x: TILE_MIN, + max_x: TILE_MAX, + min_y: TILE_MIN, + max_y: TILE_MAX, };
yud0uhu
force-pushed
the
fix/wireframe
branch
from
8d79093 to
ad4563b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
nusamai/src/sink/cesiumtiles/slice.rs (2)
18-23: 構造体のドキュメントと派生トレイトを追加することを推奨タイル境界を表す構造体の理解を深めるため、以下の改善を提案します：
+/// タイルの境界を表す構造体 +/// +/// # フィールド +/// * `min_x` - X座標の最小値 +/// * `max_x` - X座標の最大値 +/// * `min_y` - Y座標の最小値 +/// * `max_y` - Y座標の最大値 +#[derive(Debug, Clone, Copy)] struct TileBounds { min_x: f64, max_x: f64, min_y: f64, max_y: f64, }
406-411: マジックナンバーの説明が必要タイル境界の初期化に使用されている0.0から1.0の値は正規化されたタイル座標を表していますが、これが明確ではありません。以下のような改善を提案します：
+ // タイル座標を0.0から1.0の範囲に正規化 let tile_bounds = TileBounds { min_x: 0.0, max_x: 1.0, min_y: 0.0, max_y: 1.0, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
nusamai/src/sink/cesiumtiles/slice.rs(3 hunks)
🔇 Additional comments (2)
nusamai/src/sink/cesiumtiles/slice.rs (2)
413-427: 頂点のスナップ処理が正しく実装されています
以前の問題点が解決され、以下の点が改善されています：
- リングのコピーを作成して反復処理
- スナップされた頂点を正しく
poly_bufに反映 - 空のリングに対する適切なハンドリング
464-481: 🛠️ Refactor suggestion
浮動小数点の比較精度とテストの追加が必要
前回のレビューで指摘された改善点がまだ実装されていません。以下の修正を推奨します：
+/// 頂点をタイル境界にスナップする
+///
+/// # 引数
+/// * `vertex` - スナップ対象の頂点 [x, y, z, u, v]
+/// * `tile_bounds` - タイル境界
+///
+/// # 戻り値
+/// スナップされた頂点
fn snap_to_tile_boundary(vertex: [f64; 5], tile_bounds: &TileBounds) -> [f64; 5] {
+ const EPSILON: f64 = 1e-10;
let mut snapped_vertex = vertex;
- if vertex[0] < tile_bounds.min_x {
+ if vertex[0] < tile_bounds.min_x + EPSILON {
snapped_vertex[0] = tile_bounds.min_x;
- } else if vertex[0] > tile_bounds.max_x {
+ } else if vertex[0] > tile_bounds.max_x - EPSILON {
snapped_vertex[0] = tile_bounds.max_x;
}
// Snap y coordinate
- if vertex[1] < tile_bounds.min_y {
+ if vertex[1] < tile_bounds.min_y + EPSILON {
snapped_vertex[1] = tile_bounds.min_y;
- } else if vertex[1] > tile_bounds.max_y {
+ } else if vertex[1] > tile_bounds.max_y - EPSILON {
snapped_vertex[1] = tile_bounds.max_y;
}
snapped_vertex
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn test_snap_to_tile_boundary() {
+ let bounds = TileBounds {
+ min_x: 0.0,
+ max_x: 1.0,
+ min_y: 0.0,
+ max_y: 1.0,
+ };
+
+ // 境界内の頂点はそのまま
+ let vertex = [0.5, 0.5, 1.0, 0.0, 0.0];
+ assert_eq!(snap_to_tile_boundary(vertex, &bounds), vertex);
+
+ // 境界外の頂点はスナップされる
+ let vertex = [-0.1, 1.1, 1.0, 0.0, 0.0];
+ assert_eq!(
+ snap_to_tile_boundary(vertex, &bounds),
+ [0.0, 1.0, 1.0, 0.0, 0.0]
+ );
+
+ // イプシロン付近の頂点のテスト
+ let vertex = [0.0 + 1e-11, 1.0 - 1e-11, 1.0, 0.0, 0.0];
+ assert_eq!(
+ snap_to_tile_boundary(vertex, &bounds),
+ [0.0, 1.0, 1.0, 0.0, 0.0]
+ );
+ }
+}
yud0uhu
force-pushed
the
fix/wireframe
branch
from
ad4563b to
78f90be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
nusamai/src/sink/cesiumtiles/slice.rs (1)
18-23: 構造体のドキュメントと検証機能の追加を推奨以下の改善を提案します：
- 構造体の目的と各フィールドの説明を追加
- 境界値の検証メソッドの実装（min <= max）
+/// タイルの境界を表す構造体 +/// +/// # フィールド +/// * `min_x` - X座標の最小値 +/// * `max_x` - X座標の最大値 +/// * `min_y` - Y座標の最小値 +/// * `max_y` - Y座標の最大値 struct TileBounds { min_x: f64, max_x: f64, min_y: f64, max_y: f64, } + +impl TileBounds { + /// 境界値が有効か検証する + fn is_valid(&self) -> bool { + self.min_x <= self.max_x && self.min_y <= self.max_y + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
nusamai/src/sink/cesiumtiles/slice.rs(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
nusamai/src/sink/cesiumtiles/slice.rs
[warning] 417-417: nusamai/src/sink/cesiumtiles/slice.rs#L417
Added line #L417 was not covered by tests
[warning] 469-469: nusamai/src/sink/cesiumtiles/slice.rs#L469
Added line #L469 was not covered by tests
[warning] 476-476: nusamai/src/sink/cesiumtiles/slice.rs#L476
Added line #L476 was not covered by tests
| let tile_bounds = TileBounds { | ||
| min_x: 0.0, | ||
| max_x: 1.0, | ||
| min_y: 0.0, | ||
| max_y: 1.0, | ||
| }; | ||
|
|
||
| let rings: Vec<_> = poly_buf.rings().map(|r| r.raw_coords().to_vec()).collect(); | ||
|
|
||
| for ring in rings.iter() { | ||
| if ring.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| ring_buffer.clear(); | ||
|
|
||
| for vertex in ring { | ||
| let snapped_vertex = snap_to_tile_boundary(*vertex, &tile_bounds); | ||
| ring_buffer.push(snapped_vertex); | ||
| } | ||
| let drained_ring: Vec<_> = ring_buffer.drain(..).collect(); | ||
| poly_buf.add_ring(drained_ring); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
スナッピング処理の最適化を推奨
現在の実装では、以下の点で非効率的です：
- 不必要なベクターのコピー（
to_vec()） - 二重のメモリ確保（
ring_bufferとdrained_ring）
以下の最適化を提案します：
- let rings: Vec<_> = poly_buf.rings().map(|r| r.raw_coords().to_vec()).collect();
-
- for ring in rings.iter() {
+ let rings: Vec<_> = poly_buf.rings().collect();
+ poly_buf.clear();
+
+ for ring in rings {
if ring.is_empty() {
continue;
}
ring_buffer.clear();
- for vertex in ring {
- let snapped_vertex = snap_to_tile_boundary(*vertex, &tile_bounds);
+ for vertex in ring.raw_coords() {
+ let snapped_vertex = snap_to_tile_boundary(vertex, &tile_bounds);
ring_buffer.push(snapped_vertex);
}
- let drained_ring: Vec<_> = ring_buffer.drain(..).collect();
- poly_buf.add_ring(drained_ring);
+ poly_buf.add_ring(ring_buffer.drain(..));
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 417-417: nusamai/src/sink/cesiumtiles/slice.rs#L417
Added line #L417 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
nusamai/src/sink/cesiumtiles/slice.rs (1)
18-23: 構造体のドキュメントを追加することを推奨タイル境界を表現する重要な構造体なので、以下のようなドキュメントの追加を推奨します：
+/// タイルの境界を表現する構造体 +/// +/// # フィールド +/// * `min_x` - X座標の最小値 +/// * `max_x` - X座標の最大値 +/// * `min_y` - Y座標の最小値 +/// * `max_y` - Y座標の最大値 struct TileBounds { min_x: f64, max_x: f64, min_y: f64, max_y: f64, }
| let rings: Vec<_> = poly_buf.rings().map(|r| r.raw_coords().to_vec()).collect(); | ||
|
|
||
| for ring in rings.iter() { | ||
| if ring.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| ring_buffer.clear(); | ||
|
|
||
| for vertex in ring { | ||
| let snapped_vertex = snap_to_tile_boundary(*vertex, &tile_bounds); | ||
| ring_buffer.push(snapped_vertex); | ||
| } | ||
| let drained_ring: Vec<_> = std::mem::take(&mut ring_buffer).drain(..).collect(); | ||
| poly_buf.add_ring(drained_ring); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
スナッピング処理の最適化が必要
現在の実装には以下の非効率な点があります：
- 不必要なベクターのコピー（
to_vec()） - 二重のメモリ確保（
ring_bufferとdrained_ring）
以下の最適化を提案します：
- let rings: Vec<_> = poly_buf.rings().map(|r| r.raw_coords().to_vec()).collect();
-
- for ring in rings.iter() {
+ let rings: Vec<_> = poly_buf.rings().collect();
+ poly_buf.clear();
+
+ for ring in rings {
if ring.is_empty() {
continue;
}
ring_buffer.clear();
- for vertex in ring {
- let snapped_vertex = snap_to_tile_boundary(*vertex, &tile_bounds);
+ for vertex in ring.raw_coords() {
+ let snapped_vertex = snap_to_tile_boundary(vertex, &tile_bounds);
ring_buffer.push(snapped_vertex);
}
- let drained_ring: Vec<_> = ring_buffer.drain(..).collect();
- poly_buf.add_ring(drained_ring);
+ poly_buf.add_ring(ring_buffer.drain(..));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rings: Vec<_> = poly_buf.rings().map(|r| r.raw_coords().to_vec()).collect(); | |
| for ring in rings.iter() { | |
| if ring.is_empty() { | |
| continue; | |
| } | |
| ring_buffer.clear(); | |
| for vertex in ring { | |
| let snapped_vertex = snap_to_tile_boundary(*vertex, &tile_bounds); | |
| ring_buffer.push(snapped_vertex); | |
| } | |
| let drained_ring: Vec<_> = std::mem::take(&mut ring_buffer).drain(..).collect(); | |
| poly_buf.add_ring(drained_ring); | |
| } | |
| let rings: Vec<_> = poly_buf.rings().collect(); | |
| poly_buf.clear(); | |
| for ring in rings { | |
| if ring.is_empty() { | |
| continue; | |
| } | |
| ring_buffer.clear(); | |
| for vertex in ring.raw_coords() { | |
| let snapped_vertex = snap_to_tile_boundary(vertex, &tile_bounds); | |
| ring_buffer.push(snapped_vertex); | |
| } | |
| poly_buf.add_ring(ring_buffer.drain(..)); | |
| } |
| fn snap_to_tile_boundary(vertex: [f64; 5], tile_bounds: &TileBounds) -> [f64; 5] { | ||
| let mut snapped_vertex = vertex; | ||
|
|
||
| if vertex[0] < tile_bounds.min_x { | ||
| snapped_vertex[0] = tile_bounds.min_x; | ||
| } else if vertex[0] > tile_bounds.max_x { | ||
| snapped_vertex[0] = tile_bounds.max_x; | ||
| } | ||
|
|
||
| // Snap y coordinate | ||
| if vertex[1] < tile_bounds.min_y { | ||
| snapped_vertex[1] = tile_bounds.min_y; | ||
| } else if vertex[1] > tile_bounds.max_y { | ||
| snapped_vertex[1] = tile_bounds.max_y; | ||
| } | ||
|
|
||
| snapped_vertex | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
スナッピング関数の改善が必要
以下の改善を推奨します：
- 浮動小数点の精度問題に対処するためのイプシロン値の追加
- 関数のドキュメント追加
- テストケースの追加
+/// 頂点をタイル境界にスナップする
+///
+/// # 引数
+/// * `vertex` - スナップ対象の頂点 [x, y, z, u, v]
+/// * `tile_bounds` - タイル境界
+///
+/// # 戻り値
+/// スナップされた頂点
fn snap_to_tile_boundary(vertex: [f64; 5], tile_bounds: &TileBounds) -> [f64; 5] {
+ const EPSILON: f64 = 1e-10;
let mut snapped_vertex = vertex;
- if vertex[0] < tile_bounds.min_x {
+ if vertex[0] < tile_bounds.min_x + EPSILON {
snapped_vertex[0] = tile_bounds.min_x;
- } else if vertex[0] > tile_bounds.max_x {
+ } else if vertex[0] > tile_bounds.max_x - EPSILON {
snapped_vertex[0] = tile_bounds.max_x;
}
// Snap y coordinate
- if vertex[1] < tile_bounds.min_y {
+ if vertex[1] < tile_bounds.min_y + EPSILON {
snapped_vertex[1] = tile_bounds.min_y;
- } else if vertex[1] > tile_bounds.max_y {
+ } else if vertex[1] > tile_bounds.max_y - EPSILON {
snapped_vertex[1] = tile_bounds.max_y;
}
snapped_vertex
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn snap_to_tile_boundary(vertex: [f64; 5], tile_bounds: &TileBounds) -> [f64; 5] { | |
| let mut snapped_vertex = vertex; | |
| if vertex[0] < tile_bounds.min_x { | |
| snapped_vertex[0] = tile_bounds.min_x; | |
| } else if vertex[0] > tile_bounds.max_x { | |
| snapped_vertex[0] = tile_bounds.max_x; | |
| } | |
| // Snap y coordinate | |
| if vertex[1] < tile_bounds.min_y { | |
| snapped_vertex[1] = tile_bounds.min_y; | |
| } else if vertex[1] > tile_bounds.max_y { | |
| snapped_vertex[1] = tile_bounds.max_y; | |
| } | |
| snapped_vertex | |
| } | |
| /// 頂点をタイル境界にスナップする | |
| /// | |
| /// # 引数 | |
| /// * `vertex` - スナップ対象の頂点 [x, y, z, u, v] | |
| /// * `tile_bounds` - タイル境界 | |
| /// | |
| /// # 戻り値 | |
| /// スナップされた頂点 | |
| fn snap_to_tile_boundary(vertex: [f64; 5], tile_bounds: &TileBounds) -> [f64; 5] { | |
| const EPSILON: f64 = 1e-10; | |
| let mut snapped_vertex = vertex; | |
| if vertex[0] < tile_bounds.min_x + EPSILON { | |
| snapped_vertex[0] = tile_bounds.min_x; | |
| } else if vertex[0] > tile_bounds.max_x - EPSILON { | |
| snapped_vertex[0] = tile_bounds.max_x; | |
| } | |
| // Snap y coordinate | |
| if vertex[1] < tile_bounds.min_y + EPSILON { | |
| snapped_vertex[1] = tile_bounds.min_y; | |
| } else if vertex[1] > tile_bounds.max_y - EPSILON { | |
| snapped_vertex[1] = tile_bounds.max_y; | |
| } | |
| snapped_vertex | |
| } |
Close #627
What I did（変更内容）
Notes（連絡事項）
None / なし