Skip to content
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.

Sign up for GitHub

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

【wip】ポリゴンの頂点をタイル境界にスナップする #678

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

【wip】ポリゴンの頂点をタイル境界にスナップする #678

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
464fab1
wip: ポリゴンの頂点をタイル境界にスナップする
yud0uhu Nov 7, 2024
c57ba4d
Merge branch 'main' into fix/wireframe
yud0uhu Nov 7, 2024
8b4acae
fix: tileset path
yud0uhu Nov 7, 2024
78f90be
スナップ処理後の頂点がpoly_bufに反映されるように修正
yud0uhu Nov 7, 2024
2560841
fix: ring_bufferの内容をstd::mem::takeを使用してdrainするように修正
yud0uhu Nov 21, 2024
a91ffb6
Merge branch 'main' into fix/wireframe
yud0uhu Dec 31, 2024
656b736
refactor: remove unused TileBounds struct and snap_to_tile_boundary f…
yud0uhu Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion demo/cesium/3dtiles.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@
viewer.scene.globe.depthTestAgainstTerrain = true;

const tileset = await Cesium.Cesium3DTileset.fromUrl(
"/examples/3dtiles/tileset.json"
"./examples/3dtiles_textured/tileset.json",
{ enableDebugWireframe: true }
);
viewer.scene.primitives.add(tileset);
viewer.zoomTo(tileset);
Expand Down
53 changes: 50 additions & 3 deletions nusamai/src/sink/cesiumtiles/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@

use super::{material::Material, tiling};
use crate::sink::cesiumtiles::{material::Texture, tiling::zxy_from_lng_lat};

struct TileBounds {
min_x: f64,
max_x: f64,
min_y: f64,
max_y: f64,
}
#[derive(Serialize, Deserialize)]
pub struct SlicedFeature {
// polygons [x, y, z, u, v]
Expand Down Expand Up @@ -258,7 +263,7 @@
// todo?: check interior bbox to optimize

for (ri, (ring, uv_ring)) in poly.rings().zip_eq(poly_uv.rings()).enumerate() {
if ring.raw_coords().is_empty() {
if ring.is_empty() {
continue;
}

Expand Down Expand Up @@ -345,7 +350,7 @@
poly_buf.clear();

for ring in y_sliced_poly.rings() {
if ring.raw_coords().is_empty() {
if ring.is_empty() {
continue;
}

Expand Down Expand Up @@ -398,6 +403,29 @@

poly_buf.add_ring(ring_buffer.drain(..))
}
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;

Check warning on line 417 in nusamai/src/sink/cesiumtiles/slice.rs

View check run for this annotation

Codecov / codecov/patch

nusamai/src/sink/cesiumtiles/slice.rs#L417 

Added line #L417 was not covered by tests
}

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);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

スナッピング処理の最適化が必要

現在の実装には以下の非効率な点があります：

  1. 不必要なベクターのコピー（to_vec()）
  2. 二重のメモリ確保（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.

Suggested change
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(..));
}


send_polygon(key, &poly_buf);
}
Expand Down Expand Up @@ -433,3 +461,22 @@
entry_lod == selected_lod
}
}

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;

Check warning on line 469 in nusamai/src/sink/cesiumtiles/slice.rs

View check run for this annotation

Codecov / codecov/patch

nusamai/src/sink/cesiumtiles/slice.rs#L469 

Added line #L469 was not covered by tests
} 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;

Check warning on line 476 in nusamai/src/sink/cesiumtiles/slice.rs

View check run for this annotation

Codecov / codecov/patch

nusamai/src/sink/cesiumtiles/slice.rs#L476 

Added line #L476 was not covered by tests
} else if vertex[1] > tile_bounds.max_y {
snapped_vertex[1] = tile_bounds.max_y;
}

snapped_vertex
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

スナッピング関数の改善が必要

以下の改善を推奨します：

  1. 浮動小数点の精度問題に対処するためのイプシロン値の追加
  2. 関数のドキュメント追加
  3. テストケースの追加
+/// 頂点をタイル境界にスナップする
+///
+/// # 引数
+/// * `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.

Suggested change
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
}