テクスチャ付き最大LODを出力するオプションを追加 #647
Conversation
Walkthroughこのプルリクエストでは、複数のファイルにおいて構造体やメソッドの変更が行われています。主な変更点は、 Changes
Assessment against linked issues
Possibly related issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
Codecov ReportAttention: Patch coverage is Additional details and impacted files
📢 Thoughts on this report? Let us know! |
![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 (2)
nusamai/src/transformer/setting.rs (2)
142-143: テストケースの追加が必要です。追加された行がテストでカバーされていないようです。Boolean パラメータタイプの処理ロジックを実装する際は、適切なテストケースを追加してください。
テストコードの生成やGitHub issueの作成をお手伝いしましょうか?
Tools
GitHub Check: codecov/patch
[warning] 142-143: nusamai/src/transformer/setting.rs#L142-L143
Added lines #L142 - L143 were not covered by tests
163-169: 変更内容に問題ありません。テストケースの追加をお願いします。新しい "textured_max_lod" オプションの処理が正しく実装されています。LODフィルターモードを
TexturedHighestに設定し、appearanceをtrueに設定することで、期待される動作が実現できています。ただし、追加された行がテストでカバーされていないようです。動作を確認するためのテストケースを追加してください。
テストコードの生成やGitHub issueの作成をお手伝いしましょうか?
Tools
GitHub Check: codecov/patch
[warning] 163-169: nusamai/src/transformer/setting.rs#L163-L169
Added lines #L163 - L169 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- nusamai-plateau/src/entity.rs (1 hunks)
- nusamai/src/sink/cesiumtiles/mod.rs (1 hunks)
- nusamai/src/sink/gltf/mod.rs (1 hunks)
- nusamai/src/sink/obj/mod.rs (1 hunks)
- nusamai/src/sink/obj/obj_writer.rs (1 hunks)
- nusamai/src/transformer/setting.rs (3 hunks)
- nusamai/src/transformer/transform/lods.rs (3 hunks)
Files skipped from review due to trivial changes (1)
- nusamai/src/sink/obj/obj_writer.rs
Additional context used
GitHub Check: codecov/patch
nusamai-plateau/src/entity.rs
[warning] 8-8: nusamai-plateau/src/entity.rs#L8
Added line #L8 was not covered by testsnusamai/src/transformer/setting.rs
[warning] 142-143: nusamai/src/transformer/setting.rs#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 163-169: nusamai/src/transformer/setting.rs#L163-L169
Added lines #L163 - L169 were not covered by testsnusamai/src/transformer/transform/lods.rs
[warning] 35-37: nusamai/src/transformer/transform/lods.rs#L35-L37
Added lines #L35 - L37 were not covered by tests
[warning] 39-40: nusamai/src/transformer/transform/lods.rs#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 42-42: nusamai/src/transformer/transform/lods.rs#L42
Added line #L42 was not covered by tests
[warning] 44-54: nusamai/src/transformer/transform/lods.rs#L44-L54
Added lines #L44 - L54 were not covered by tests
[warning] 57-59: nusamai/src/transformer/transform/lods.rs#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: nusamai/src/transformer/transform/lods.rs#L61
Added line #L61 was not covered by tests
[warning] 64-64: nusamai/src/transformer/transform/lods.rs#L64
Added line #L64 was not covered by tests
[warning] 69-71: nusamai/src/transformer/transform/lods.rs#L69-L71
Added lines #L69 - L71 were not covered by tests
[warning] 83-84: nusamai/src/transformer/transform/lods.rs#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 86-89: nusamai/src/transformer/transform/lods.rs#L86-L89
Added lines #L86 - L89 were not covered by tests
[warning] 91-91: nusamai/src/transformer/transform/lods.rs#L91
Added line #L91 was not covered by tests
Additional comments not posted (7)
nusamai-plateau/src/entity.rs (1)
8-8: LGTM!
CloneトレイトをEntity構造体の derive マクロに追加することは、構造体のインスタンスをクローンできるようにするための適切な変更です。クローンは、データ構造を複製する必要がある場合によく使用されます。この変更は、コードの既存のロジックやコントロールフローを変更するものではありません。
追加された行がテストでカバーされていないことについては、
Cloneトレイトは標準的な Rust の機能であり、その動作は明確に定義されているため、重大な問題ではありません。ただし、将来的にはテストケースを追加することを検討してください。Tools
GitHub Check: codecov/patch
[warning] 8-8: nusamai-plateau/src/entity.rs#L8
Added line #L8 was not covered by testsnusamai/src/transformer/setting.rs (2)
71-75: LGTM!新しいオプション "テクスチャ付き最大LOD" が正しく追加されています。ラベルと値のフォーマットが既存のオプションと一致しており、問題ありません。
163-169: 新しいオプションが期待通りに動作することを確認してください。以下のスクリプトを実行して、"textured_max_lod" オプションが正しく動作することを確認してください。
Verification successful
新しい "textured_max_lod" オプションが正しく実装されていることを確認しました。
コードベースの分析結果から、以下のことが確認できました：
- "textured_max_lod" オプションが
nusamai/src/transformer/setting.rsファイルに正しく実装されています。- このオプションが選択された場合、
LodFilterMode::TexturedHighestが設定されます。- 同時に
set_appearance(true)が呼び出され、appearanceが正しく設定されます。LodFilterMode::TexturedHighestはnusamai/src/transformer/transform/lods.rsファイルでも参照されており、適切に統合されていることが示唆されます。これらの結果から、新しいオプションが期待通りに実装され、動作することが確認できました。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 説明: "textured_max_lod" オプションが正しく動作することを確認します。 # テスト: "textured_max_lod" オプションを選択した場合の動作を確認します。 # 期待される結果: # - LODフィルターモードが `TexturedHighest` に設定されること。 # - appearanceがtrueに設定されること。 # TransformerRegistryのbuildメソッドをモック化してテストを実行 cat <<EOF | cargo test use crate::transformer::setting::{LodSelection, ParameterType, Selection, TransformerConfig, TransformerRegistry}; use crate::sink::DataRequirements; use crate::transformer::{LodFilterMode, LodFilterSpec}; #[test] fn test_textured_max_lod_option() { let mut registry = TransformerRegistry::new(); let lod_selection = Selection::new(LodSelection::get_lod_selection_options(), "textured_max_lod"); let config = TransformerConfig { key: "use_lod".to_string(), label: "".to_string(), parameter: ParameterType::Selection(lod_selection), }; registry.insert(config); let requirements = registry.build(DataRequirements::default()); assert_eq!(requirements.lod_filter.mode, LodFilterMode::TexturedHighest); assert_eq!(requirements.appearance, true); } EOFLength of output: 92
Script:
#!/bin/bash # Search for the "textured_max_lod" option implementation ast-grep --lang rust --pattern 'match $_ { $$$ "textured_max_lod" => { $$$ } $$$ }' # Search for references to LodFilterMode::TexturedHighest rg "LodFilterMode::TexturedHighest" -n # Search for set_appearance(true) calls rg "set_appearance\(true\)" -nLength of output: 499
Tools
GitHub Check: codecov/patch
[warning] 163-169: nusamai/src/transformer/setting.rs#L163-L169
Added lines #L163 - L169 were not covered by testsnusamai/src/sink/gltf/mod.rs (1)
32-32: テクスチャ設定の削除は適切です。テクスチャ設定
use_texture_config(false)の削除により、トランスフォーマーオプションが簡素化されています。この変更は、GLTFデータシンクにおけるテクスチャの処理や利用方法に影響を与える可能性があります。nusamai/src/sink/cesiumtiles/mod.rs (1)
41-41: テクスチャ設定オプションの削除は適切です。
use_texture_config(false)の削除により、Cesium タイルのデータ処理パイプラインが簡素化され、テクスチャ付きの最大LODを出力するオプションの導入に合致しています。この変更は、テクスチャ設定がこのコンテキストで不要になったことを示唆しています。nusamai/src/sink/obj/mod.rs (1)
39-39: LGTM!
use_lod_config関数のインポートは適切です。この関数は、おそらくObjSinkProvider構造体のtransformer_optionsメソッドで使用されます。nusamai/src/transformer/transform/lods.rs (1)
15-15:TexturedHighestモードの追加
LodFilterModeに新しいバリアントTexturedHighestを追加することで、テクスチャ付きの最高LODを選択できるようになっており、機能拡張として適切です。
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
nusamai/src/transformer/setting.rs (1)
143-144: TODOコメントに対処することを忘れないでください。このTODOコメントは、ブール型パラメータの処理ロジックがまだ実装されていないことを示しています。実装を完了させるか、適切なチケットを作成して追跡してください。
このタスクのチケットを作成しましょうか?
nusamai/src/transformer/transform/lods.rs (2)
36-36: TODO: 処理の最適化TODOコメントにあるように、処理の最適化の余地があります。パフォーマンスが重要な場合は、別のPRで最適化に取り組むことをお勧めします。
91-91:match文での網羅性の向上
match self.modeにおいて、デフォルトの_ => {}パターンがありますが、LodFilterModeの全てのバリアントを明示的にマッチさせることで、将来新しいバリアントが追加された際にコンパイラが未処理のケースを警告します。_ => {}を削除することをお勧めします。修正案：
} - _ => {} }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- nusamai/src/transformer/setting.rs (3 hunks)
- nusamai/src/transformer/transform/lods.rs (3 hunks)
Additional comments not posted (7)
nusamai/src/transformer/setting.rs (2)
71-76: LGTM!テクスチャ付き最大LODオプションの追加は良いと思います。将来の3Dタイルシンクでの全LODオプションのサポートについてのコメントも適切です。
164-170: LGTM!テクスチャ付き最大LODパラメータ値の処理ケースの追加は適切です。LODフィルターモードを
TexturedHighestに設定し、appearanceをtrueに設定することは、テクスチャ付き最大LODを処理するのに適しています。nusamai/src/transformer/transform/lods.rs (5)
15-16: LGTM!
LodFilterMode列挙型に新しいバリアントTexturedHighestとAllを追加することで、機能が拡張されています。命名規則に従っており、既存のコードを壊すこともありません。
35-73: テクスチャの有無に基づいてLODを選択する新しいロジックを追加
TexturedHighestモードでは、テクスチャ付きのLODがある場合はそのLODのエンティティを、テクスチャがない場合は最高のLODのエンティティを選択するロジックが実装されています。コードの可読性も高く、LODの選択処理が適切に行われています。
93-95:Allモードの実装
Allモードでは、フィルタリングを行わずに元のエンティティをそのまま出力しています。シンプルで分かりやすい実装になっています。
149-149:LodMaskにDebugトレイトを追加
LodMask構造体がDebugトレイトを導出するようになりました。これにより、LodMaskのインスタンスをデバッグしやすくなります。既存の機能に悪影響はありません。
49-52:unwrap()の使用によるパニックの可能性を回避する
appearance_store.read().unwrap()を使用していますが、ロックがポイズンされている場合にパニックを引き起こす可能性があります。エラーハンドリングを追加して、ロック取得に失敗した場合に適切に対処することをお勧めします。修正案：
- let appearance = entity_copy.appearance_store.read().unwrap(); + let appearance = match entity_copy.appearance_store.read() { + Ok(appearance) => appearance, + Err(e) => { + // ロック取得に失敗した場合の処理（例: エラーを記録してスキップ） + eprintln!("Failed to acquire read lock: {}", e); + return; // または適切なエラー処理 + } + };Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
nusamai/src/option.rs (1)
10-12: 関数名の変更を検討してください
use_lod_config関数は現在、テクスチャなしのLOD選択に特化しています。関数名をより明確にするために、use_non_textured_lod_configなどに変更することを検討してください。それ以外の変更は適切で、PRの目的に沿っています。テクスチャ付きとテクスチャなしのLOD選択を分離することで、コードの明確性が向上しています。
nusamai/src/transformer/setting.rs (1)
83-89: テクスチャなしLOD選択の実装が適切です。最適化の提案があります。
lod_selection_without_texture関数は、"textured_max_lod"オプションを適切にフィルタリングしています。イテレータメソッドの使用も効率的です。パフォーマンスを少し向上させるために、以下の最適化を検討してください：
- let options = Self::get_lod_selection_options() - .into_iter() + let options = Self::get_lod_selection_options() .into_iter() .filter(|&(_, value)| value != "textured_max_lod") - .collect::<Vec<_>>(); - Selection::new(options, default_value) + Selection::new(options.collect(), default_value)この変更により、中間のベクターの作成を避けることができます。
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
app/src-tauri/tauri.conf.jsonis excluded by!**/*.json
Files selected for processing (5)
- nusamai/src/option.rs (1 hunks)
- nusamai/src/sink/cesiumtiles/mod.rs (2 hunks)
- nusamai/src/sink/gltf/mod.rs (2 hunks)
- nusamai/src/sink/obj/mod.rs (2 hunks)
- nusamai/src/transformer/setting.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- nusamai/src/sink/obj/mod.rs
Additional comments not posted (6)
nusamai/src/option.rs (1)
Line range hint
1-24: 全体的な評価：良好な変更このファイルの変更は、PRの目的であるテクスチャ付き最大LODを出力するオプションの追加に適切に対応しています。テクスチャ付きとテクスチャなしのLOD選択を分離することで、コードの明確性と柔軟性が向上しています。
提案した小さな改善点（関数名の変更と一意のキーの使用）を検討し、実装することで、コードの品質がさらに向上するでしょう。
全体として、これらの変更は有益であり、プロジェクトの機能を拡張するという目標を達成しています。
nusamai/src/transformer/setting.rs (2)
79-82: テクスチャ付きLOD選択の実装が適切です。
lod_selection_with_texture関数は、テクスチャ付きのLODオプションを含むすべてのLOD選択肢を提供しており、要件を正しく満たしています。
71-76: LODオプションの追加が適切に実装されています。"テクスチャ付き最大LOD"オプションの追加はPRの目的に合致しており、適切に実装されています。
コメントアウトされている"すべてのLOD"オプションについて、以下の確認をお願いします：
この実装は3dtilesシンクに関連しているようですが、現在の開発状況や今後の計画について教えていただけますか？
Verification successful
コメントアウトされた「すべてのLOD」オプションと関連するTODOについて確認しました。
"すべてのLOD"オプションは現在コメントアウトされており、3dtiles sinkに関連するTODOが存在します。今後の実装予定に従って処理されることを確認しました。
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: 3dtilesシンクの実装状況を確認 rg --type rust "3dtiles.*sink" -C 5Length of output: 2482
Script:
#!/bin/bash # Description: Check if 'lod_all' option is implemented or if TODOs related to 3dtiles sink are addressed rg --type rust "lod_all|TODO.*3dtiles sink" -C 5Length of output: 1990
nusamai/src/sink/gltf/mod.rs (3)
32-32: 新しい設定オプションのインポートが追加されました。
use_textured_lod_configのインポートは、PRの目的であるテクスチャ付き最大LODを出力するオプションの追加と一致しています。この変更は適切であり、新しい機能の実装を支援します。
61-61: LODとテクスチャの設定が統合されました。
use_textured_lod_config("max_lod")の使用は、PRの目的に沿ってテクスチャ付き最大LODを設定するための良い方法です。この変更はコードを簡素化し、可読性を向上させています。ただし、以下の点を確認してください：
- この変更が、LODとテクスチャの設定を別々に行う必要があるケースに影響を与えていないか。
- "max_lod"以外の引数でこの関数を呼び出す必要がある場合の挙動。
以下のスクリプトを実行して、
use_textured_lod_configの使用箇所と引数を確認してください：
Line range hint
1-1000: テクスチャ関連の機能の総合的なテストを推奨します。新しい
use_textured_lod_config関数の導入に伴い、既存のテクスチャ処理ロジックとの互換性を確認することが重要です。特に以下の点に注意してテストを行ってください：
- テクスチャのパッキングプロセス（特に
TexturePackerの使用箇所）- UV座標の更新処理
- アトラステクスチャの生成と使用
これらのテストにより、新しい設定オプションが既存の機能を損なうことなく、期待通りに動作することを確認できます。
テクスチャ関連の主要な処理を特定するために、以下のスクリプトを実行してください：
Verification successful
テクスチャ関連の機能が正しく統合されています。特に
use_textured_lod_config関数は既存のテクスチャ処理ロジックと互換性があります。Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify key texture-related processing in the file rg --type rust -e 'TexturePacker' -e 'uv_coords' -e 'atlas' nusamai/src/sink/gltf/mod.rsLength of output: 1176
Script:
#!/bin/bash # Description: Search for all usages of `use_textured_lod_config` in the codebase rg --type rust 'use_textured_lod_config' nusamai/src/sink/gltf/mod.rsLength of output: 167
Script:
#!/bin/bash # Description: Find the definition and implementation of `use_textured_lod_config` in the codebase rg --type rust 'fn use_textured_lod_config' nusamaiLength of output: 150
Script:
#!/bin/bash # Description: Extract the complete definition of `use_textured_lod_config` from option.rs rg --type rust -A 20 'fn use_textured_lod_config' nusamai/src/option.rsLength of output: 393
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
nusamai/src/transformer/setting.rs (1)
151-152: 未実装のBoolean型パラメータ処理があります
ParameterType::Booleanの処理が未実装です。必要であれば、実装をお手伝いできます。実装のコードを提供しましょうか？それとも、GitHub Issueを作成してこのタスクを追跡しますか？
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- nusamai/src/transformer/setting.rs (3 hunks)
Additional comments not posted (4)
nusamai/src/transformer/setting.rs (4)
71-76: 新しいLODオプションの追加が適切です「テクスチャ付き最大LOD」オプションが正しく追加されています。
79-82:lod_selection_with_texture関数の実装が正しいです新しいLODオプションを含めた選択肢が正しく作成されています。
83-89:lod_selection_without_texture関数の実装が正しいです「テクスチャ付き最大LOD」オプションを除外するフィルタリングが正しく行われています。
172-178:"textured_max_lod"オプションの処理が適切です
data_requirementsに対してLodFilterMode::TexturedHighestを設定し、set_appearance(true)が正しく呼び出されています。
There was a problem hiding this comment.
@satoshi7190 要求は満たしているためapproveですが、質問への回答と整理・整頓が必須かなと思いました…！
マージはちょっと保留…
いや、マージはします！問題を切り分けてちゃんと対処した方が良いと思ったので！
| @@ -5,7 +5,7 @@ use nusamai_citygml::{geometry::GeometryStore, object::Value}; | |||
| use crate::appearance::AppearanceStore; | |||
|
|
|||
| /// City objects, features, objects or data | |||
| #[derive(Debug, serde::Deserialize, serde::Serialize)] | |||
| #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] | |||
There was a problem hiding this comment.
q) 結局ここのCloneは除去できなかった感じですかね…
There was a problem hiding this comment.
僕が提案したコードを適用すると、除去できる気はしています！
| @@ -92,7 +146,7 @@ fn find_lods(value: &Value) -> LodMask { | |||
| mask | |||
| } | |||
|
|
|||
| #[derive(Default, Clone, Copy)] | |||
| #[derive(Default, Clone, Copy, Debug)] | |||
There was a problem hiding this comment.
nits) Debugは消しときましょうか！
| out.push(entity); | ||
| match self.mode { | ||
| LodFilterMode::TexturedHighest => { | ||
| // TODO: Processing needs to be optimised |
There was a problem hiding this comment.
nits) Processing needs to be optimized.
nits) 「最適化」とは具体的に何をするのか、記載しておいた方が良いかと！
| edit_tree(&mut entity.root, target_lod); | ||
| out.push(entity); | ||
| match self.mode { | ||
| LodFilterMode::TexturedHighest => { |
There was a problem hiding this comment.
imo) テクスチャが付いた最大のLODを抽出する。テクスチャがない場合は最大のLODを抽出する。
くらいのコメントはあった方が良いと思いました！
| match self.mode { | ||
| LodFilterMode::TexturedHighest => { | ||
| // TODO: Processing needs to be optimised | ||
| let original_lods = find_lods(&entity.root) & self.mask; |
There was a problem hiding this comment.
fyi) シンプルに、こんな感じではいかがでしょう。
一応、自分で動かしてみたところ、動作はしているように見えました。
let available_lods = find_lods(&entity.root) & self.mask;
let mut highest_textured_lod = None;
// 「最大のLOD」は初めから決まっている。テクスチャが存在しなければ、即座に最大LODを返却できる
let highest_available_lod = available_lods.highest_lod().unwrap_or(0);
// revで逆順イテレータを作成
for lod in (0..=highest_available_lod).rev() {
if available_lods.0 & (1 << lod) != 0 {
edit_tree(&mut entity.root, lod);
let has_textures = {
let appearance = entity.appearance_store.read().unwrap();
!appearance.textures.is_empty()
};
// テクスチャが存在するLODが見つかれば、それを保存して終了
if has_textures {
highest_textured_lod = Some(lod);
break;
}
}
}
// highest_textured_lodがNoneではないならhighest_textured_lodを利用
// Noneならhighest_available_lodを利用
if let Some(lod) = highest_textured_lod.or(Some(highest_available_lod)) {
edit_tree(&mut entity.root, lod);
out.push(entity);
}| let original_lods = find_lods(&entity.root) & self.mask; | |
| let original_lods = find_lods(&entity.root) & self.mask; |
| } | ||
| } | ||
|
|
||
| pub fn use_texture_config(default_value: bool) -> TransformerConfig { | ||
| pub fn use_textured_lod_config(default_value: &str) -> TransformerConfig { |
There was a problem hiding this comment.
q) use_lod_configとuse_textured_lod_configで、use_lodという同名キーが利用されているのが全く理解できず…
オプションは分ける必要がある感じでしょうか？
| @@ -7,16 +7,18 @@ pub fn use_lod_config(default_value: &str) -> TransformerConfig { | |||
| TransformerConfig { | |||
There was a problem hiding this comment.
q) 前提がよく理解できていなかったんですが、option.rsはSinkのオプションで、transformer/setting.rsはトランスフォーマーのオプションを列挙している感じでしょうか？
There was a problem hiding this comment.
option.rsが transformerフォルダに属していないのが気になりました。
There was a problem hiding this comment.
imo) optionなのか、configなのか、settingなのか…表記が揺れていて、かつどれも異なる意味を持っていそうで、理解に時間がかかっちゃいそうです…！
整理が必要かと…
#643 （最適化が必要なのでクローズしません）
Close #644
Close #645
※不要なコミットが多いため
Squash and mergeを推奨What I did（変更内容）
-t use_lod=textured_max_lod）-t use_texture=true）Notes（連絡事項）
動作確認