-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
テクスチャスケーリング処理をglTFおよびOBJ処理にも導入する #631
Conversation
Walkthroughこの変更は、テクスチャ解像度の制御を強化するために、 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GltfSinkProvider
participant GltfSink
participant ObjSinkProvider
participant ObjSink
participant TextureResolution
User->>GltfSinkProvider: Create with params
GltfSinkProvider->>GltfSink: Pass limit_texture_resolution
GltfSink->>TextureResolution: Calculate downsample scale
User->>ObjSinkProvider: Create with params
ObjSinkProvider->>ObjSink: Pass limit_texture_resolution
ObjSink->>TextureResolution: Calculate downsample scale
Assessment against linked issues
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- nusamai/src/sink/cesiumtiles/mod.rs (2 hunks)
- nusamai/src/sink/gltf/mod.rs (4 hunks)
- nusamai/src/sink/mod.rs (1 hunks)
- nusamai/src/sink/obj/mod.rs (5 hunks)
- nusamai/src/sink/texture_resolution.rs (1 hunks)
Additional comments not posted (15)
nusamai/src/sink/texture_resolution.rs (2)
1-2
: テクスチャ解像度の制限に関する定数の追加頂点間の距離(メートル)に基づいてテクスチャ解像度を制限するための定数
MAX_TEXTURE_PIXELS_PER_METER
が追加されました。この定数はテクスチャのピクセル密度を制御するために使用されます。
17-53
: ポリゴンのテクスチャダウンサンプルスケールを計算する関数の追加この関数は、ポリゴンの頂点データとテクスチャサイズに基づいてテクスチャのダウンサンプルスケールを計算します。
limit_texture_resolution
パラメータを使用して、テクスチャ解像度の制限が有効かどうかを判断し、適切なスケールを返します。この関数はテクスチャの効率的な処理に寄与します。nusamai/src/sink/mod.rs (1)
16-16
: 新しいモジュールtexture_resolution
の追加テクスチャ解像度を管理または処理する機能を拡張するために、
texture_resolution
モジュールが新たに追加されました。このモジュールの追加は、テクスチャの取り扱いを改善することを目的としています。nusamai/src/sink/gltf/mod.rs (5)
39-40
: テクスチャ解像度の制御関数のインポート
get_texture_downsample_scale_of_polygon
関数をsuper::texture_resolution
モジュールからインポートしています。この関数はテクスチャのダウンサンプルスケールを計算するために使用され、GltfSink
の機能を強化します。
76-84
: 新しいパラメータlimit_texture_resolution
の定義
GltfSinkProvider
のparameters
メソッドにlimit_texture_resolution
パラメータが追加されました。このパラメータはテクスチャ解像度を制限するために使用され、オプショナルです。パラメータの説明には「距離(メートル)あたりのテクスチャの解像度を制限する」と記載されています。
104-111
:GltfSink
構造体のインスタンス作成時にlimit_texture_resolution
パラメータを使用
create
メソッド内でlimit_texture_resolution
パラメータの値を取得し、GltfSink
構造体のインスタンス作成時にこの値を渡しています。これにより、テクスチャ解像度の制限機能がGltfSink
に組み込まれます。
119-119
:GltfSink
構造体に新しいフィールドlimit_texture_resolution
の追加
GltfSink
構造体にlimit_texture_resolution
フィールドが新たに追加されました。このフィールドはオプションのブール型で、テクスチャ解像度の制限を管理します。
469-474
: テクスチャのダウンサンプルスケールを計算するロジックの更新
get_texture_downsample_scale_of_polygon
関数を使用して、テクスチャのダウンサンプルスケールを計算し、その結果をdownsample_scale
変数に格納しています。この計算は、オリジナルの頂点とテクスチャサイズに基づいて行われ、limit_texture_resolution
パラメータが影響を与えます。nusamai/src/sink/cesiumtiles/mod.rs (3)
49-49
: 新しい関数のインポートを確認してください。
get_texture_downsample_scale_of_polygon
関数がsuper::texture_resolution
モジュールから正しくインポートされています。この変更は承認されました。
Line range hint
93-102
: 新しいパラメータの追加を確認してください。
limit_texture_resolution
パラメータが追加され、テクスチャの解像度を制限するオプションが提供されています。これはPRの目的に沿っています。この変更は承認されました。
534-538
: テクスチャ解像度の制限の統合を確認してください。
get_texture_downsample_scale_of_polygon
関数が新しいlimit_texture_resolution
パラメータを使用してテクスチャのダウンサンプルスケールを計算しています。この統合が正しく機能しているか確認することをお勧めします。nusamai/src/sink/obj/mod.rs (4)
46-46
: 新しい関数のインポートを確認してください。
get_texture_downsample_scale_of_polygon
関数がsuper::texture_resolution
モジュールから正しくインポートされています。この変更は承認されました。
93-102
: 新しいパラメータの追加を確認してください。
limit_texture_resolution
パラメータが追加され、テクスチャの解像度を制限するオプションが提供されています。これはPRの目的に沿っています。この変更は承認されました。
121-130
: テクスチャ解像度の制限の統合を確認してください。
create
メソッドが新しいlimit_texture_resolution
パラメータを取得し、ObjSink
構造体に渡しています。この統合が正しく機能しているか確認することをお勧めします。
504-510
: テクスチャ解像度の制限の統合を確認してください。
get_texture_downsample_scale_of_polygon
関数が新しいlimit_texture_resolution
パラメータを使用してテクスチャのダウンサンプルスケールを計算しています。この統合が正しく機能しているか確認することをお勧めします。Verification successful
テクスチャ解像度の制限の統合が正しく機能しています。
get_texture_downsample_scale_of_polygon
関数は、limit_texture_resolution
パラメータを使用して、テクスチャのダウンサンプルスケールを適切に調整しています。この統合は正しく機能しています。Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # テクスチャ解像度の制限が正しく機能しているかのテストコードを実行します。 echo "テストコードをここに記述してください。"Length of output: 258
Script:
#!/bin/bash # Search for the implementation of the `get_texture_downsample_scale_of_polygon` function ast-grep --lang rust --pattern 'fn get_texture_downsample_scale_of_polygon($_, $_, $_) { $$$ }'Length of output: 97
Script:
#!/bin/bash # Use ripgrep to search for the definition of the `get_texture_downsample_scale_of_polygon` function rg 'fn get_texture_downsample_scale_of_polygon' -A 10Length of output: 914
Script:
#!/bin/bash # Extract more lines from the `get_texture_downsample_scale_of_polygon` function to understand its implementation rg 'fn get_texture_downsample_scale_of_polygon' -A 30Length of output: 2440
Script:
#!/bin/bash # Extract more lines from the `get_texture_downsample_scale_of_polygon` function to capture the complete logic rg 'fn get_texture_downsample_scale_of_polygon' -A 50Length of output: 2829
// WARN: This function has an equivalent in `atlas-packer/src/texture.rs`. | ||
fn uv_to_pixel_coords(uv_coords: &[(f64, f64)], width: u32, height: u32) -> Vec<(u32, u32)> { | ||
uv_coords | ||
.iter() | ||
.map(|(u, v)| { | ||
( | ||
(u.clamp(0.0, 1.0) * width as f64).min(width as f64 - 1.0) as u32, | ||
((1.0 - v.clamp(0.0, 1.0)) * height as f64).min(height as f64 - 1.0) as u32, | ||
) | ||
}) | ||
.collect() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UV座標からピクセル座標への変換関数の追加
この関数はUV座標をピクセル座標に変換します。ただし、atlas-packer/src/texture.rs
に同等の機能が存在するため、コードの重複に注意が必要です。可能であれば、重複を避けるために共通のライブラリやモジュールを使用することを検討してください。
コードの重複を解消するためのリファクタリングを提案します。
-// WARN: This function has an equivalent in `atlas-packer/src/texture.rs`.
+// TODO: Consider refactoring to use a common library for UV to pixel coordinate conversion.
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.
// WARN: This function has an equivalent in `atlas-packer/src/texture.rs`. | |
fn uv_to_pixel_coords(uv_coords: &[(f64, f64)], width: u32, height: u32) -> Vec<(u32, u32)> { | |
uv_coords | |
.iter() | |
.map(|(u, v)| { | |
( | |
(u.clamp(0.0, 1.0) * width as f64).min(width as f64 - 1.0) as u32, | |
((1.0 - v.clamp(0.0, 1.0)) * height as f64).min(height as f64 - 1.0) as u32, | |
) | |
}) | |
.collect() | |
} | |
// TODO: Consider refactoring to use a common library for UV to pixel coordinate conversion. | |
fn uv_to_pixel_coords(uv_coords: &[(f64, f64)], width: u32, height: u32) -> Vec<(u32, u32)> { | |
uv_coords | |
.iter() | |
.map(|(u, v)| { | |
( | |
(u.clamp(0.0, 1.0) * width as f64).min(width as f64 - 1.0) as u32, | |
((1.0 - v.clamp(0.0, 1.0)) * height as f64).min(height as f64 - 1.0) as u32, | |
) | |
}) | |
.collect() | |
} |
Codecov ReportAttention: Patch coverage is Additional details and impacted files
📢 Thoughts on this report? Let us know! |
@@ -13,6 +13,7 @@ pub mod obj; | |||
pub mod ply; | |||
pub mod serde; | |||
pub mod shapefile; | |||
mod texture_resolution; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: Sinkか、そうではないモジュールか、でコメントを入れておいた方がいいかもですね。
@@ -0,0 +1,53 @@ | |||
/// Limits the texture resolution based on the distance (meters) between the vertices of a polygon. | |||
const MAX_TEXTURE_PIXELS_PER_METER: f64 = 30.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo: よければテストを作成していただけると大変ありがたいです!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
モジュール内に書き込むテストで十分です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TadaTeruki lgtm!
軽微なコメントをしています!
Close #626
What I did(変更内容)
Notes(連絡事項)