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.

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

3D Tilesのglbファイルをgzipで圧縮した状態で表示するオプションを追加 #672

Merged
merged 4 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
26 changes: 21 additions & 5 deletions nusamai/src/sink/cesiumtiles/gltf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use ahash::{HashMap, HashSet};
use byteorder::{ByteOrder, LittleEndian};
use flate2::{write::GzEncoder, Compression};
use indexmap::IndexSet;
use nusamai_gltf_json::extensions::mesh::ext_mesh_features;

Expand All @@ -16,6 +17,7 @@

pub type Primitives = HashMap<material::Material, PrimitiveInfo>;

#[allow(clippy::too_many_arguments)]
pub fn write_gltf_glb<W: Write>(
feedback: &feedback::Feedback,
writer: W,
Expand All @@ -24,6 +26,7 @@
primitives: Primitives,
num_features: usize,
metadata_encoder: MetadataEncoder,
gzip_compress: bool,
) -> Result<(), PipelineError> {
use nusamai_gltf_json::*;

Expand Down Expand Up @@ -281,12 +284,25 @@
..Default::default()
};

// Write glb to the writer
nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).unwrap().into(),
bin: Some(bin_content.into()),
if gzip_compress {
// Write glb to the writer with gzip compression
let mut encoder = GzEncoder::new(writer, Compression::default());

nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).unwrap().into(),
bin: Some(bin_content.into()),
}
.to_writer_with_alignment(&mut encoder, 8)?;

Check warning on line 295 in nusamai/src/sink/cesiumtiles/gltf.rs

View check run for this annotation

Codecov / codecov/patch

nusamai/src/sink/cesiumtiles/gltf.rs#L289-L295

Added lines #L289 - L295 were not covered by tests

encoder.finish()?;

Check warning on line 297 in nusamai/src/sink/cesiumtiles/gltf.rs

View check run for this annotation

Codecov / codecov/patch

nusamai/src/sink/cesiumtiles/gltf.rs#L297

Added line #L297 was not covered by tests
} else {
// Write glb to the writer
nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).unwrap().into(),
bin: Some(bin_content.into()),
}
.to_writer_with_alignment(writer, 8)?;
Comment on lines +287 to +304
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. コードの重複を減らす

以下のような実装を提案します:

-    if gzip_compress {
-        // Write glb to the writer with gzip compression
-        let mut encoder = GzEncoder::new(writer, Compression::default());
-
-        nusamai_gltf::glb::Glb {
-            json: serde_json::to_vec(&gltf).unwrap().into(),
-            bin: Some(bin_content.into()),
-        }
-        .to_writer_with_alignment(&mut encoder, 8)?;
-
-        encoder.finish()?;
-    } else {
-        // Write glb to the writer
-        nusamai_gltf::glb::Glb {
-            json: serde_json::to_vec(&gltf).unwrap().into(),
-            bin: Some(bin_content.into()),
-        }
-        .to_writer_with_alignment(writer, 8)?;
-    }
+    let glb = nusamai_gltf::glb::Glb {
+        json: serde_json::to_vec(&gltf).map_err(|e| PipelineError::SerializationError(e.to_string()))?.into(),
+        bin: Some(bin_content.into()),
+    };
+
+    let mut writer = if gzip_compress {
+        Box::new(GzEncoder::new(writer, Compression::default())) as Box<dyn Write>
+    } else {
+        Box::new(writer) as Box<dyn Write>
+    };
+
+    glb.to_writer_with_alignment(&mut writer, 8)?;
+
+    if gzip_compress {
+        writer.flush()?;
+    }

この改善により:

  • エラーハンドリングが強化されます
  • コードの重複が減ります
  • より明確な制御フローになります
📝 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
if gzip_compress {
// Write glb to the writer with gzip compression
let mut encoder = GzEncoder::new(writer, Compression::default());
nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).unwrap().into(),
bin: Some(bin_content.into()),
}
.to_writer_with_alignment(&mut encoder, 8)?;
encoder.finish()?;
} else {
// Write glb to the writer
nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).unwrap().into(),
bin: Some(bin_content.into()),
}
.to_writer_with_alignment(writer, 8)?;
let glb = nusamai_gltf::glb::Glb {
json: serde_json::to_vec(&gltf).map_err(|e| PipelineError::SerializationError(e.to_string()))?.into(),
bin: Some(bin_content.into()),
};
let mut writer = if gzip_compress {
Box::new(GzEncoder::new(writer, Compression::default())) as Box<dyn Write>
} else {
Box::new(writer) as Box<dyn Write>
};
glb.to_writer_with_alignment(&mut writer, 8)?;
if gzip_compress {
writer.flush()?;
}

}
.to_writer_with_alignment(writer, 8)?;

Ok(())
}
16 changes: 16 additions & 0 deletions nusamai/src/sink/cesiumtiles/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ impl DataSinkProvider for CesiumTilesSinkProvider {
},
});
params.define(limit_texture_resolution_parameter(false));
params.define(ParameterDefinition {
key: "gzip".into(),
entry: ParameterEntry {
description: "gzip compress".into(),
required: false,
parameter: ParameterType::Boolean(BooleanParameter { value: Some(false) }),
label: Some("gzipで圧縮する".into()),
},
});

params
}
Expand All @@ -112,12 +121,14 @@ impl DataSinkProvider for CesiumTilesSinkProvider {
let max_z = get_parameter_value!(params, "max_z", Integer).unwrap() as u8;
let limit_texture_resolution =
*get_parameter_value!(params, "limit_texture_resolution", Boolean);
let gzip_compress = *get_parameter_value!(params, "gzip", Boolean);
let transform_settings = self.transformer_options();

Box::<CesiumTilesSink>::new(CesiumTilesSink {
output_path: output_path.as_ref().unwrap().into(),
transform_settings,
limit_texture_resolution,
gzip_compress,
min_z,
max_z,
})
Expand All @@ -128,6 +139,7 @@ struct CesiumTilesSink {
output_path: PathBuf,
transform_settings: TransformerRegistry,
limit_texture_resolution: Option<bool>,
gzip_compress: Option<bool>,
min_z: u8,
max_z: u8,
}
Expand Down Expand Up @@ -157,6 +169,7 @@ impl DataSink for CesiumTilesSink {
let max_zoom = self.max_z;

let limit_texture_resolution = self.limit_texture_resolution;
let gzip_compress = self.gzip_compress;

// TODO: refactoring

Expand Down Expand Up @@ -205,6 +218,7 @@ impl DataSink for CesiumTilesSink {
tile_id_conv,
schema,
limit_texture_resolution,
gzip_compress,
) {
feedback.fatal_error(error);
}
Expand Down Expand Up @@ -327,6 +341,7 @@ fn tile_writing_stage(
tile_id_conv: TileIdMethod,
schema: &Schema,
limit_texture_resolution: Option<bool>,
gzip_compress: Option<bool>,
) -> Result<()> {
let ellipsoid = nusamai_projection::ellipsoid::wgs84();
let contents: Arc<Mutex<Vec<TileContent>>> = Default::default();
Expand Down Expand Up @@ -707,6 +722,7 @@ fn tile_writing_stage(
primitives,
features.len(),
metadata_encoder,
gzip_compress.unwrap_or_default(),
)?;

Ok::<(), PipelineError>(())
Expand Down
2 changes: 1 addition & 1 deletion nusamai/src/sink/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub fn limit_texture_resolution_parameter(default_value: bool) -> ParameterDefin
parameter: ParameterType::Boolean(BooleanParameter {
value: Some(default_value),
}),
label: Some("距離(メートル)あたりのテクスチャの解像度を制限する".into()),
label: Some("距離あたりの解像度を制限する".into()),
},
}
}
Loading