整数値パラメータの追加、MVT で min_z, max_z オプションを使えるようにする #347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Walkthroughこれらの変更は、特定のファイルではなく、全体の変更の要約を提供しています。命名規則に従い、 Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ciscorn
added
Controller
ciscorn
force-pushed
the
integer-params
branch
from
8d85611 to
43f90f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- app/src-tauri/src/main.rs (2 hunks)
- nusamai/src/lib.rs (1 hunks)
- nusamai/src/parameters/mod.rs (7 hunks)
- nusamai/src/sink/mvt/mod.rs (6 hunks)
- nusamai/tests/sink.rs (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/src-tauri/src/main.rs
- nusamai/src/lib.rs
- nusamai/src/sink/mvt/mod.rs
- nusamai/tests/sink.rs
Additional comments: 7
nusamai/src/parameters/mod.rs (7)
- 120-120:
ParameterType::Integer(p) => p.validate(self.required),の行について、新しいIntegerParameter型のバリデーションロジックが正しく追加されています。これにより、整数型パラメータのバリデーションが可能になり、システムの柔軟性が向上します。- 124-130:
ParameterEntryのupdate_value_with_strメソッドにおいて、ParameterType::Integer(p) => p.update_value_with_str(s),の行が追加されています。これにより、文字列から整数型パラメータへの値の更新が可能になります。ただし、入力値のバリデーションが重要になりますので、不正な値が渡された場合のエラーハンドリングが適切に行われているか確認する必要があります。- 140-140:
ParameterEntryのupdate_value_with_jsonメソッドにおいて、ParameterType::Integer(p) => p.update_value_with_json(v),の行が追加されています。JSON値から整数型パラメータへの値の更新が可能になり、API経由での設定変更などに対応できるようになります。こちらも、不正な値が渡された場合のエラーハンドリングが適切に行われているか確認する必要があります。- 150-150:
ParameterType列挙型にInteger(IntegerParameter),が追加されています。これにより、新しい整数型パラメータがシステム全体で利用可能になり、より多様な入力値の検証と処理が可能になります。- 216-216:
StringParameterのupdate_value_with_jsonメソッドにおいて、エラーメッセージが"Value must be a string."に変更されています。この変更により、エラーメッセージがより明確になり、ユーザーが問題を理解しやすくなります。- 240-240:
BooleanParameterのupdate_value_with_strメソッドにおいて、エラーメッセージが"Value must be a boolean."に変更されています。この変更により、エラーメッセージがより明確になり、ユーザーが問題を理解しやすくなります。- 251-251:
BooleanParameterのupdate_value_with_jsonメソッドにおいて、エラーメッセージが"Value must be a boolean."に変更されています。この変更により、エラーメッセージがより明確になり、ユーザーが問題を理解しやすくなります。
Comment on lines
+256
to
+310
| #[derive(Debug, Serialize)] | ||
| pub struct IntegerParameter { | ||
| pub value: Option<i64>, | ||
| pub min: Option<i64>, | ||
| pub max: Option<i64>, | ||
| } | ||
|
|
||
| impl IntegerParameter { | ||
| pub fn validate(&self, required: bool) -> Result<(), Error> { | ||
| match self.value { | ||
| Some(v) => { | ||
| if let Some(min) = self.min { | ||
| if v < min { | ||
| return Err(Error::InvalidValue(format!( | ||
| "Value must be greater than or equal to {}.", | ||
| min | ||
| ))); | ||
| } | ||
| } | ||
| if let Some(max) = self.max { | ||
| if v > max { | ||
| return Err(Error::InvalidValue(format!( | ||
| "Value must be less than or equal to {}.", | ||
| max | ||
| ))); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| None => { | ||
| if required { | ||
| return Err(Error::RequiredValueNotProvided); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn update_value_with_str(&mut self, s: &str) -> Result<(), Error> { | ||
| let Ok(v) = s.parse::<i64>() else { | ||
| return Err(Error::InvalidValue("Value must be an integer.".into())); | ||
| }; | ||
| self.value = Some(v); | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn update_value_with_json(&mut self, v: &serde_json::Value) -> Result<(), Error> { | ||
| if let serde_json::Value::Number(n) = v { | ||
| let Some(v) = n.as_i64() else { | ||
| return Err(Error::InvalidValue("Value must be an integer.".into())); | ||
| }; | ||
| self.value = Some(v); | ||
| Ok(()) | ||
| } else { | ||
| Err(Error::InvalidValue("Value must be an integer.".into())) |
There was a problem hiding this comment.
新しく追加されたIntegerParameter構造体とそのメソッド(validate, update_value_with_str, update_value_with_json)について、整数型パラメータのバリデーション、文字列やJSONからの値の更新が適切に実装されています。特に、最小値と最大値の制約を設けることで、パラメータの範囲を制限し、システムの安全性を高めることができます。ただし、update_value_with_strとupdate_value_with_jsonメソッドでのエラーメッセージが同じであるため、ユーザーがエラーの原因を特定しにくい可能性があります。エラーメッセージに入力形式（文字列またはJSON）の情報を追加することを検討してください。
- return Err(Error::InvalidValue("Value must be an integer.".into()));
+ return Err(Error::InvalidValue("Value must be an integer in string format.".into()));- return Err(Error::InvalidValue("Value must be an integer.".into()));
+ return Err(Error::InvalidValue("Value must be an integer in JSON format.".into()));
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
📢 Thoughts on this report? Let us know! |
ciscorn
force-pushed
the
integer-params
branch
from
43f90f7 to
365fb20
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- app/src-tauri/src/main.rs (2 hunks)
- nusamai/src/lib.rs (1 hunks)
- nusamai/src/parameters/mod.rs (7 hunks)
- nusamai/src/sink/mvt/mod.rs (6 hunks)
- nusamai/tests/sink.rs (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/src-tauri/src/main.rs
- nusamai/src/lib.rs
- nusamai/src/parameters/mod.rs
- nusamai/src/sink/mvt/mod.rs
- nusamai/tests/sink.rs
ciscorn
force-pushed
the
integer-params
branch
from
365fb20 to
46405a7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- app/src-tauri/src/main.rs (2 hunks)
- nusamai/src/lib.rs (1 hunks)
- nusamai/src/main.rs (1 hunks)
- nusamai/src/parameters/mod.rs (7 hunks)
- nusamai/src/sink/mvt/mod.rs (6 hunks)
- nusamai/tests/sink.rs (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/src-tauri/src/main.rs
- nusamai/src/lib.rs
- nusamai/src/parameters/mod.rs
- nusamai/src/sink/mvt/mod.rs
- nusamai/tests/sink.rs
Additional comments: 2
nusamai/src/main.rs (2)
- 42-48:
sourceoptとsinkoptの宣言順序が入れ替わり、それぞれのコメントが更新されています。この変更は、コードの可読性を向上させ、オプションの目的をより正確に反映しています。この変更により、コマンドラインインターフェースの使用がより直感的になり、ユーザーが入力ソースと出力フォーマットのオプションを理解しやすくなります。- 39-51: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-141]
ファイル全体を通して、新しい機能や改善点が適切に統合されているか確認しました。特に、
Args構造体の変更は、ユーザーがコマンドラインから新しいオプションを指定する際の使いやすさを向上させることに貢献しています。また、SinkChoice構造体やその他の関数に関する変更も、全体のコードの整合性と機能性を保つ上で重要な役割を果たしています。このファイルには、パフォーマンスやセキュリティに関する懸念事項は見受けられませんでした。さらに、エラーハンドリングやログ出力の処理も適切に行われており、ユーザーが問題を診断しやすくなっています。
ciscorn
force-pushed
the
integer-params
branch
from
6e19cca to
46405a7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- nusamai/src/sink/cesiumtiles/gltf.rs (7 hunks)
- nusamai/src/sink/cesiumtiles/mod.rs (3 hunks)
Additional comments: 12
nusamai/src/sink/cesiumtiles/gltf.rs (8)
- 9-13:
PrimitiveInfo構造体の追加は、頂点のインデックスと機能IDを管理するための明確な方法を提供します。ただし、feature_idsをHashSetとして保持することで、挿入順序が保持されない点に注意してください。これが意図した設計であるか確認してください。- 21-21:
write_gltf_glb関数のvertices引数が[u32; 6]の配列を受け取るように変更されました。この変更は、頂点データにfeature_idを含めるためのものです。この変更により、関数の柔軟性が向上し、機能IDを含む頂点データの処理が可能になります。- 37-37: 頂点データのバイトストライドが
4 * 6に変更されています。これは、feature_idを含む新しい頂点フォーマットに対応するためです。この変更は適切であり、頂点データの構造を正確に反映しています。- 34-45: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [42-54]
頂点データの処理において、
feature_idの追加が反映されています。この変更により、GLTF出力における機能IDのサポートが可能になります。ただし、feature_idの使用方法については、ドキュメントやコメントでの補足説明があると理解が深まります。
- 88-96:
feature_idに対するアクセサの追加は、GLTF出力における機能IDのサポートを完全にするものです。この変更により、頂点データに含まれる機能IDを適切に扱うことができます。設計の意図に沿った実装であると考えられます。- 127-127:
_FEATURE_ID_0属性の追加により、GLTF出力における機能IDのサポートが強化されています。この属性を使用することで、各頂点に関連付けられた機能IDを効果的に表現できます。この変更は、GLTF出力の柔軟性と表現力を高めるものです。- 134-146:
ext_mesh_features拡張の使用により、GLTF出力における機能IDのサポートがさらに強化されています。この拡張を通じて、機能IDに関連する追加情報をGLTF出力に含めることができます。この変更は、GLTFの機能を最大限に活用し、より詳細な情報を提供するためのものです。- 208-208:
EXT_mesh_features拡張の使用を宣言しています。この宣言により、GLTFファイルがext_mesh_features拡張を使用していることが明示され、互換性のあるパーサーによる適切な処理が保証されます。この変更は、GLTF出力の互換性と拡張性を向上させるものです。nusamai/src/sink/cesiumtiles/mod.rs (4)
- 259-259:
verticesコレクションが[u32; 6]の配列を格納するように変更されました。この変更は、頂点データにfeature_idを含めるためのものです。この変更により、機能IDを含む頂点データの処理が可能になり、出力の詳細度が向上します。- 263-263:
primitivesマップの処理が更新され、各プリミティブにfeature_idsセットが含まれるようになりました。この変更により、機能IDに基づいてプリミティブをより効果的に追跡および管理できるようになります。この変更は、出力の精度と管理性を向上させるものです。- 295-296:
primitivesマップにおけるfeature_idsの追加は、各プリミティブに関連付けられた機能IDを追跡するためのものです。この変更により、GLTF出力における機能IDのサポートが強化され、より詳細な情報を提供できるようになります。この変更は、出力の柔軟性と表現力を高めるものです。- 292-306: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [303-312]
頂点データの処理において、
feature_idの追加が反映されています。この変更により、GLTF出力における機能IDのサポートが可能になります。ただし、feature_idをf32として扱っている点に注意してください。これは、GLTF仕様におけるデータ型の制約によるものかもしれませんが、意図した設計であるか確認してください。
nokonoko1203
approved these changes
Feb 27, 2024
nokonoko1203
approved these changes
Feb 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MVTSink→MvtSinkにリネームオプション値の指定方法 (CLI)：
Summary by CodeRabbit
IntegerParameter型を導入し、バリデーションと値の更新メソッドを追加しました。MvtOptions構造体を追加しました。