search

lo48576 / fbxcel-dom

FBX DOM library for Rust. // See https://github.com/lo48576/fbx-viewer for working example application // rework (total rewrite) is planned
Apache License 2.0
25 stars 10 forks source link

Adds a GlobalSettings struct. #7

Open Kleptine opened 3 years ago

Kleptine commented 3 years ago

Exposes a struct-based API for accessing GlobalSettings properties. Currently only supports unit_scale_factor and the mentioned escape hatch.

This change is Reviewable

lo48576 commented 3 years ago

I think unit_scale_factor should encode physical unit in its return type. Returning f64 would be semantically unclear and error-prone. (On the other hand, returning Option<f64> from unit_scale_factor_raw is acceptable, since it is raw value :-) )

It should return some dedicated scale type such as struct UnitScale(f64);. Defining UnitScale::to_{{some_common_physical_units}} and UnitScale::scale_factor_to_{{some_common_physical_units}}, users can do something like unit_scale.to_meter(raw_coord) or let scale_to_milimeter: f32 = unit_scale.scale_factor_to_milimeter();. In this way, both developers and readers of the downstream crate can understand the code, without the knowledge of the default physical unit in FBX.

lo48576 commented 3 years ago

Another option is, to define #[non_exhaustive] enum PhysicalUnit { Millimeter, Centimeter, Meter, /*...*/ } and pass it to conversion functions, for example unit_scale.scale_to(raw_coord, PhysicalUnit::Millimeter) or unit_scale.scale_factor_to(PhysicalUnit::Meter).

I feel the former (to_{{some_common_physical_units}}) is enough for now, but the latter would be also OK. If design issues are found later, we can change it (since this crate is still experimental stage 😓).