Skip to content

feat(stage): add Stage::meters_per_unit() -> Option<f64>#43

Closed
yohawing wants to merge 1 commit intomxpv:mainfrom
yohawing:upstream-review/stage-meters-per-unit
Closed

feat(stage): add Stage::meters_per_unit() -> Option<f64>#43
yohawing wants to merge 1 commit intomxpv:mainfrom
yohawing:upstream-review/stage-meters-per-unit

Conversation

@yohawing
Copy link
Copy Markdown
Contributor

@yohawing yohawing commented Apr 7, 2026

Summary

Adds a typed accessor for the root layer's metersPerUnit metadata:

pub fn meters_per_unit(&self) -> Option<f64>

metersPerUnit represents how many meters one scene unit corresponds to. Common values are 0.01 (centimeters, used by many DCC tools) and 1.0 (meters). This PR returns None when the metadata is not authored; the USD specification does not define a default value, so the calling application should decide (e.g. Houdini picks 1.0, Maya picks 0.01).

Companion to #42 (Stage::up_axis()) but fully independent - each PR introduces its own FieldKey, its own fixtures, and its own tests so they can be reviewed/merged in any order.

Changes

  • src/sdf/schema.rs: add FieldKey::MetersPerUnit ("metersPerUnit").
  • src/stage.rs: add Stage::meters_per_unit() accessor.
  • fixtures/meters_per_unit_centimeters.usda, meters_per_unit_meters.usda, meters_per_unit_absent.usda: minimal fixtures.
  • Tests added in stage::tests:
    • meters_per_unit_centimeters: fixture with 0.01 returns Some(0.01)
    • meters_per_unit_meters: fixture with 1.0 returns Some(1.0)
    • meters_per_unit_absent_returns_none: fixture without the field returns None

Test plan

  • cargo test passes (228 tests, 3 new)
  • cargo clippy --all-targets -- -D warnings clean
  • cargo fmt --check clean

Adds a typed accessor for the root layer's metersPerUnit metadata:

    pub fn meters_per_unit(&self) -> Option<f64>

metersPerUnit represents how many meters one scene unit corresponds
to.  Common values are 0.01 (centimeters) and 1.0 (meters).  Returns
None when the metadata is not authored; the USD specification does
not define a default value, so callers should make that decision.

Changes:
- Add FieldKey::MetersPerUnit ("metersPerUnit") to sdf::schema.
- Add Stage::meters_per_unit() accessor in stage.rs.

Tests added:
- meters_per_unit_centimeters: fixture with 0.01 returns Some(0.01).
- meters_per_unit_meters: fixture with 1.0 returns Some(1.0).
- meters_per_unit_absent_returns_none: no-metadata fixture returns None.

Fixtures added:
- fixtures/meters_per_unit_centimeters.usda
- fixtures/meters_per_unit_meters.usda
- fixtures/meters_per_unit_absent.usda
Copilot AI review requested due to automatic review settings April 7, 2026 07:43
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a typed accessor on Stage for retrieving the authored metersPerUnit metadata (scene-unit scale in meters) and introduces minimal fixtures + tests to validate present/absent behavior.

Changes:

  • Add FieldKey::MetersPerUnit ("metersPerUnit") to the Sdf schema field registry.
  • Add Stage::meters_per_unit() -> Option<f64> with rustdoc example.
  • Add three USDA fixtures and three corresponding unit tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/stage.rs Adds the meters_per_unit() accessor plus tests exercising authored and absent metadata.
src/sdf/schema.rs Registers a new FieldKey variant and maps it to "metersPerUnit".
fixtures/meters_per_unit_centimeters.usda Fixture with metersPerUnit = 0.01.
fixtures/meters_per_unit_meters.usda Fixture with metersPerUnit = 1.0.
fixtures/meters_per_unit_absent.usda Fixture without metersPerUnit to validate None.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +88 to +108
/// Returns the `metersPerUnit` metadata from the root layer, if set.
///
/// The value represents how many meters one scene unit corresponds to.
/// Common values are `0.01` (centimeters, used by many DCC tools) and
/// `1.0` (meters). Returns `None` when the metadata is not authored;
/// the USD specification does not define a default value.
///
/// # Example
///
/// ```no_run
/// use openusd::{ar::DefaultResolver, Stage};
///
/// let resolver = DefaultResolver::new();
/// let stage = Stage::open(&resolver, "scene.usda").unwrap();
/// if let Some(scale) = stage.meters_per_unit() {
/// println!("1 unit = {scale} meters");
/// }
/// ```
pub fn meters_per_unit(&self) -> Option<f64> {
self.field::<f64>(&Path::abs_root(), FieldKey::MetersPerUnit).ok()?
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The docstring says this returns metersPerUnit "from the root layer" and returns None when the metadata is not authored, but the implementation uses Stage::field which resolves the strongest authored opinion across the whole layer stack. As a result, if the root layer doesn't author metersPerUnit but a sublayer does, this will return Some(...) (and conversion errors are also silently mapped to None via .ok()?). Please either (a) update the docs to describe the composed/strongest-opinion behavior, or (b) change the implementation to query only self.layers[0] so it truly reflects the root layer only (and add a test for the chosen semantics).

Copilot uses AI. Check for mistakes.
@yohawing
Copy link
Copy Markdown
Contributor Author

yohawing commented Apr 7, 2026

Hi @mxpv,

Apologies — this PR (and the other four in this batch: #41 #42 #43 #44 #45) was opened by an AI agent on my behalf without enough human review of upstream etiquette. I should have filed an issue first to discuss before dropping a stack of code on you. Sorry for the noise.

For context: I'm building yw-look, a Windows quick-look style asset inspector for CG workflows, and I need a few small accessors on top of openusd to surface stage metadata (upAxis, metersPerUnit) and detect broken references in user assets. These five PRs are the minimal set of additions I needed.

If the API and approach look reasonable to you, I'd be grateful if you could merge them. If not — totally fine to close, I'd just maintain them on my fork instead. Happy to revise based on any feedback.

Thanks for the great crate!

@mxpv
Copy link
Copy Markdown
Owner

mxpv commented Apr 7, 2026

This will be included in UsdGeom module, which is planned. I'm going to close this one for now, as it's easy to workaround. See details thoughts in #42 (comment)

@mxpv mxpv closed this Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants