国土基本図郭の範囲取得実装追加 #271
国土基本図郭の範囲取得実装追加 #271
Conversation
gitignore cmakelists.txt gitignore clangd grid_code.h standard map grid GridCode::create grid実装中 同上 ビルド通る C++テスト通る fix c wrapper gmlfile::gridCode 同上 以降途中 移行中
filter移行中 filter_by_grid_coords 移行中 C#テストが通る
# Conflicts: # include/plateau/dataset/gml_file.h # test/test_mesh_extractor.cpp
…ateau into feature/add_standard_map_grid
…ure/add_standard_map_grid
Walkthroughこの変更では、 Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant StandardMapGrid
participant GeoReference
Tester->>StandardMapGrid: コンストラクタでグリッドコードを渡す
StandardMapGrid->>StandardMapGrid: コードをパースし、レベル・インデックスを設定
Tester->>StandardMapGrid: getExtent() を呼び出す
StandardMapGrid->>StandardMapGrid: calculateGridExtent() で矩形座標範囲を計算
StandardMapGrid->>GeoReference: 矩形座標から地理座標へ変換
GeoReference-->>StandardMapGrid: 地理座標範囲を返す
StandardMapGrid-->>Tester: Extent を返す
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
…ure/add_standard_map_grid
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
include/plateau/dataset/standard_map_grid.h (2)
57-62:calculateGridExtent()にはnoexceptと簡潔な戻り値型 alias を
距離計算は throw しない設計方針に見えるためnoexcept指定で最適化を狙えます。
またstd::pair<TVec3d, TVec3d>は長いのでusing PlaneExtent = std::pair<TVec3d, TVec3d>;を typedef し、宣言を読みやすくすることを推奨します。
68-75: プライベートメンバの命名一貫性を保つ
first_row_ / first_column_とsecond_row_ / second_column_の並び順が逆転しています（row→column / column→row）。処理時の混乱を避けるため順序を統一してください。test/test_standard_map_grid.cpp (2)
12-33: 共通アサーションをヘルパー関数へ抽出すると可読性向上
同一ロジックの範囲包含チェックが複数回コピペされています。auto within = [](const Extent& e, const GeoCoordinate& g){ return e.max.latitude >= g.latitude && e.max.longitude >= g.longitude && e.min.latitude <= g.latitude && e.min.longitude <= g.longitude; }; ASSERT_TRUE(within(extent, expected));のようにまとめると、重複コード削減と失敗メッセージ自動生成に役立ちます。
303-331: 同じstd::coutログが大量に出力され CI ログが読みづらくなる恐れ
デバッグ用であれば#ifdef DEBUGガード、またはSCOPED_TRACEへ統合して下さい。src/dataset/standard_map_grid.cpp (4)
44-71:parseLevel()は長さのみで判定しているため Level500 と Level1000 の曖昧さが残る
8 文字コードが数字終端なら 500, 文字終端なら 1000 という仕様ですが、
std::isalpha(code.back())だけでは大文字小文字判定や全角混入を拾えません。[A-E]か[0-9]を正規表現で厳密チェックすると安全です。
170-181: Level1000 パースの境界チェックで負値を取り得ないので不要
third_column_ = 4 - digitで digit∈[0,4] なのにthird_column_ < 0は常に false。代入前に範囲外を判定するか、演算式をdigitのまま扱う方が明瞭です。
183-187: Level500 のインデックス取りはstd::stoi(code_.substr(6,1))が例外を投げ得る
事前にisdigitで確認してからc - '0'で数値化すれば try/catch が不要で高速になります。
320-325:isSmallerThanNormalGml()の比較演算子は読み手に誤解を与える
level_ < Level2500と記述されていますが「小さい＝詳細度が低い／高い」どちらを指すか曖昧です。メソッド名をisCoarserThanNormalGml()などに変更するかドキュメントで定義を明示してください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/plateau/dataset/standard_map_grid.h(2 hunks)src/dataset/standard_map_grid.cpp(1 hunks)test/CMakeLists.txt(1 hunks)test/test_standard_map_grid.cpp(1 hunks)
🔇 Additional comments (4)
include/plateau/dataset/standard_map_grid.h (1)
10-11: enum の前方宣言は OK だが cpp 側の定義と名前空間が一致しているか再確認を
cpp ではenum class StandardMapGridLevelを再定義しています。ヘッダと cpp が同一名前空間で同一スコープに置かれていないと ODR 違反になる恐れがあります。現状は一致していますが、将来的に名前空間を変更した場合に壊れるため、ヘッダ側で定義まで行い cpp 側でusingする構成も検討してください。test/test_standard_map_grid.cpp (1)
127-145:upper()の戻り値でisValid()==falseになるケースをテスト
現状 Level50000 → upper() だけを検証していますが、Level500 から多段で連続upper()を呼んだ場合の動作も追加すると境界確認になります。src/dataset/standard_map_grid.cpp (2)
135-142:second_column_ = 9 - std::stoi(code_.substr(4,1));の座標系注釈が逆
コメントに「基点は左下（90）」とありますが実装は9 - nで左上基点になります。ドキュメントと実装を合わせるか、変換式を見直してください。
198-216:row_index = first_row_ - 'E'の基準点選択が不自然
グリッド仕様上、東西 A‑H を 0‑7 とするほうが理解しやすく、計算もfirst_row_ - 'A'で済みます。意図的ならコメントで理由を補足してください。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/dataset/standard_map_grid.cpp (2)
94-100: 文字判定での安全な実装
std::isalnumの代わりに明示的にASCII範囲チェック(c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z')を使用している点が適切です。これにより、全角英数字などの意図しない文字を確実に除外できます。
275-286:upperRawメソッドの適切な実装上位レベルのグリッドコードを生成する際に、単に文字列を切り詰めるだけでなく、新しいインスタンスを作成してパース処理を行う実装は適切です。これにより内部状態の整合性が保たれます。
🧹 Nitpick comments (3)
test/test_standard_map_grid.cpp (2)
94-95: コメントの説明がテスト内容と一致していません。コメントに「Level2500」と書かれていますが、この関数名は「Level1000_WithinBounds」で、Level1000のテストを行っています。コメントを以下のように修正してください。
- const auto extent = StandardMapGrid("08JE640E").getExtent(); // Level2500 + const auto extent = StandardMapGrid("08JE640E").getExtent(); // Level1000
176-179: デバッグ用の出力はテスト完了後に削除することを検討してください。
std::coutを使ったデバッグ出力がテストコード内に残されています。テストが安定したら、これらの出力は削除するか、#ifdef DEBUGなどで囲むことを検討してください。src/dataset/standard_map_grid.cpp (1)
176-183: 例外処理と詳細なエラーログの実装パース処理中の例外をキャッチし、詳細なエラーメッセージをログに出力する実装は、デバッグ時に非常に役立ちます。ただし、本番環境では
std::cerrへのログ出力を制御するための仕組み（ログレベルなど）も検討すると良いでしょう。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/plateau/dataset/standard_map_grid.h(2 hunks)src/dataset/standard_map_grid.cpp(1 hunks)test/CMakeLists.txt(1 hunks)test/test_standard_map_grid.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/CMakeLists.txt
- include/plateau/dataset/standard_map_grid.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/dataset/standard_map_grid.cpp (5)
include/plateau/dataset/standard_map_grid.h (13)
StandardMapGrid(26-85)StandardMapGrid(28-28)StandardMapGrid(29-29)get(34-34)calculateGridExtent(71-71)getExtent(39-39)isValid(44-44)upper(49-49)upperRaw(50-50)getLevel(55-55)isLargestLevel(60-60)isSmallerThanNormalGml(61-61)isNormalGmlLevel(62-62)src/dataset/mesh_code.cpp (18)
get(259-287)get(259-259)getExtent(139-191)getExtent(139-139)isValid(293-295)isValid(293-293)upper(243-247)upper(243-243)upperRaw(249-257)upperRaw(249-249)getLevel(289-291)getLevel(289-289)isLargestLevel(297-299)isLargestLevel(297-297)isSmallerThanNormalGml(301-303)isSmallerThanNormalGml(301-301)isNormalGmlLevel(305-307)isNormalGmlLevel(305-305)include/plateau/dataset/grid_code.h (9)
get(24-24)getExtent(29-29)isValid(34-34)upper(39-39)upperRaw(44-44)getLevel(49-49)isLargestLevel(54-54)isSmallerThanNormalGml(59-59)isNormalGmlLevel(64-64)src/dataset/gml_file.cpp (2)
isValid(111-113)isValid(111-111)src/geometry/geo_reference.cpp (1)
GeoReference(6-12)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test (macos-14, arm64)
- GitHub Check: build-and-test (windows-2022, x86_64)
🔇 Additional comments (9)
test/test_standard_map_grid.cpp (5)
171-174: 浮動小数点の比較にEXPECT_NEARを使用しているのは良い実装です。座標変換における丸め誤差を考慮して
EXPECT_NEARを使用し、適切な許容誤差（0.1）を設定している点が適切です。
12-57: 階層化された包括的なテストケースが実装されています。各レベルの地図グリッド（50000, 5000, 2500, 1000）に対する境界チェックが適切に実装されており、地理座標変換の正確性を確認するための堅牢なテストになっています。各グリッドレベルに対して期待される座標点が含まれているかどうかを検証する手法は効果的です。
110-116: 無効なグリッドコードの検証が網羅的です。無効なコードパターン（無効な文字、長さの問題）に対するテストが適切に実装されています。これによりパーサーの堅牢性が向上します。
127-145: 階層変換の検証が明確です。
upper()メソッドの動作を各レベルから上位レベルへの変換として検証しており、特に最上位レベルからの変換が無効になることを確認している点が良いです。
165-208: 直交座標での範囲計算テストが適切に実装されています。Level50000の基準点および南北東西方向のオフセットケースのテストが網羅的に行われています。基準点（
08JE）、南側（08NA）、北側（08FA）の3つのケースをテストしている点が優れています。src/dataset/standard_map_grid.cpp (4)
12-87: 定数とヘルパー関数の適切な分離と設計namespace内に階層構造に応じた定数（セルサイズ、分割数）を明確に定義し、ヘルパー関数を適切に実装している点が優れています。特に
calculateSubGridExtent関数はコードの重複を避けるために効果的に利用されています。
190-244:calculateGridExtentメソッドの実装が堅牢平面直角座標系での範囲計算が階層的に実装され、各レベルに対して適切な計算が行われています。特に基準点からのオフセット計算と、レベルに応じた再帰的な範囲の絞り込みが明確に実装されている点が優れています。
246-263:getExtentメソッドでの座標変換が適切
calculateGridExtentで得られた平面直角座標をGeoReferenceを使用して緯度経度に変換する実装が適切です。特に座標系のゾーンIDを図郭コードから取得して使用している点が重要です。
288-302: レベル関連メソッドの明確な実装
getLevel,isLargestLevel,isSmallerThanNormalGml,isNormalGmlLevelなどのメソッドが、内部のレベル列挙型に基づいて適切に実装されています。これにより論理的な一貫性が保たれています。
|
LGTM |
関連リンク
https://synesthesias.atlassian.net/browse/CPSDK25-59
こちらの対応
実装内容
レビュー前確認項目
マージ前確認項目
動作確認
その他
Summary by CodeRabbit
新機能
バグ修正
テスト
その他