blender/blender
177
547
Fork 822
Code Issues 6.6k Pull Requests 995 Projects 21 Wiki Activity

USD: Add an option to convert the scene's meters per unit value #122804

Open
Charles Wardlaw wants to merge 13 commits from CharlesWardlaw/blender:feature/usd_convert_to_cm into main
pull from: CharlesWardlaw/blender:feature/usd_convert_to_cm
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 5 Commits 13 Files Changed 6 +151 -2
Charles Wardlaw commented 2024-06-05 22:17:22 +02:00
Member
Copy Link

This rescales the whole scene by its root transform to match the same visual size while not forcing the user to wait for scale to be applied to each object.

This is requested by studios whose main applications / USD scenes are in CM, because referencing and payloading scenes from disparate scales can cause issues at resolution time.

If "Apply Unit Scale Conversion" is unchecked on import, the user now has the ability to bring the objects in with a scale factor of 1.0, so that the objects may be edited as if Blender's scene units matches the imported stage's.

At export time, a "Stage Meters Per Unit" value can be chosen from a list of common measurements, as well as setting a custom value.

This rescales the whole scene by its root transform to match the same visual size while not forcing the user to wait for scale to be applied to each object. This is requested by studios whose main applications / USD scenes are in CM, because referencing and payloading scenes from disparate scales can cause issues at resolution time. If "Apply Unit Scale Conversion" is unchecked on import, the user now has the ability to bring the objects in with a scale factor of 1.0, so that the objects may be edited as if Blender's scene units matches the imported stage's. At export time, a "Stage Meters Per Unit" value can be chosen from a list of common measurements, as well as setting a custom value.
Charles Wardlaw added 1 commit 2024-06-05 22:17:33 +02:00
Charles Wardlaw requested review from Michael Kowalski 2024-06-05 22:41:50 +02:00
Charles Wardlaw requested review from Jesse Yurkovich 2024-06-05 22:42:02 +02:00
Michael Kowalski commented 2024-06-05 22:47:38 +02:00
Member
Copy Link

Thanks for porting this @CharlesWardlaw ! I was thinking that the unit conversion should be made more generic to handle arbitrary unit scaling, not just centimeters. The initial USD branch implementation supported centimeters only, but I always felt that this prototype is too limited in that regard. It shouldn't be hard to extend this to handle other units. What do you think?

Thanks for porting this @CharlesWardlaw ! I was thinking that the unit conversion should be made more generic to handle arbitrary unit scaling, not just centimeters. The initial USD branch implementation supported centimeters only, but I always felt that this prototype is too limited in that regard. It shouldn't be hard to extend this to handle other units. What do you think?
Dhruv Govil commented 2024-06-05 22:52:33 +02:00
First-time contributor
Copy Link

I agree that the functionality would be useful (even though we prefer meters ourselves).

However, I think this might be better served as a dropdown enum choice of common units? With a default of Meters, and any other unit sizes as options (could just be meters and centimetres to start). In fact, I'd recommend that the input to the operator itself actually be a float that defaults to 1 (metersPerUnit=1.0) and the dropdown just be a UI set of preset values.

That way it's generalizable and easy to add more unit sizes in the future, since some studios use imperial units. By generalizing it to a scalar , that enum just needs to change the scalar without modifying the rest of the code.

This is generally how I implemented it in other DCCs and our internal pipeline as well.

I agree that the functionality would be useful (even though we prefer meters ourselves). However, I think this might be better served as a dropdown enum choice of common units? With a default of Meters, and any other unit sizes as options (could just be meters and centimetres to start). In fact, I'd recommend that the input to the operator itself actually be a float that defaults to 1 (metersPerUnit=1.0) and the dropdown just be a UI set of preset values. That way it's generalizable and easy to add more unit sizes in the future, since some studios use imperial units. By generalizing it to a scalar , that enum just needs to change the scalar without modifying the rest of the code. This is generally how I implemented it in other DCCs and our internal pipeline as well.
👍 1
Charles Wardlaw commented 2024-06-05 23:14:43 +02:00
Author
Member
Copy Link

It's easy enough to add-- I'll send a follow up in a few.

It's easy enough to add-- I'll send a follow up in a few.
Charles Wardlaw added 1 commit 2024-06-06 20:45:10 +02:00
Charles Wardlaw commented 2024-06-06 20:45:43 +02:00
Author
Member
Copy Link

I think this is correct, but I'm going to do a bunch more tests first. 🤣

I think this is correct, but I'm going to do a bunch more tests first. 🤣
Charles Wardlaw changed title from USD: Adding a Convert to Centimeters option to USD: Adding an option to convert the scene's meters per unit value 2024-06-06 20:48:17 +02:00
Charles Wardlaw added this to the USD project 2024-07-11 23:15:35 +02:00
Michael Kowalski reviewed 2024-07-12 20:23:55 +02:00
source/blender/editors/io/io_usd.cc Outdated
@ -159,0 +164,4 @@
{USD_SCENE_UNITS_INCHES, "INCHES", 0, "Inches", "Scene scale of 0.0254"},
{USD_SCENE_UNITS_FEET, "FEET", 0, "Feet", "Scene scale of 0.3048"},
{USD_SCENE_UNITS_YARDS, "YARDS", 0, "Yards", "Scene scale of 0.9144"},
{USD_SCENE_UNITS_CUSTOM, "CUSTOM", 0, "Custom", "Specify a custom scene scale value"},
Michael Kowalski commented 2024-07-12 20:23:55 +02:00
Member
Copy Link

Minor point: the help for the Custom option is "Specify a custom scene scale value". However, the custom value is actually in meters-per-unit. Would it be better to be consistent? I.e., either have the help for all the menu items be in meters-per-unit, or have the Custom option be a scene scale? The former option (using meters-per-unit for all items) would probably be less work and might fit better with the USD conventions. Again, this might not be an important issue.

Minor point: the help for the `Custom` option is "Specify a custom scene scale value". However, the custom value is actually in meters-per-unit. Would it be better to be consistent? I.e., either have the help for all the menu items be in meters-per-unit, or have the `Custom` option be a scene scale? The former option (using meters-per-unit for all items) would probably be less work and might fit better with the USD conventions. Again, this might not be an important issue.
Charles Wardlaw commented 2024-12-17 01:11:32 +01:00
Author
Member
Copy Link

Good point-- I've reworked the math to make those numbers make sense as meters per unit values, and changed the Custom values to be in meters per unit. Benefit of this is it made me rethink some terrible floating point conversions. 🤦

Good point-- I've reworked the math to make those numbers make sense as meters per unit values, and changed the Custom values to be in meters per unit. Benefit of this is it made me rethink some terrible floating point conversions. 🤦
CharlesWardlaw marked this conversation as resolved
Michael Kowalski reviewed 2024-07-12 22:29:37 +02:00
source/blender/io/usd/intern/usd_writer_transform.cc
@ -45,1 +45,3 @@
if (!usd_export_context_.export_params.convert_orientation) {
if ((!usd_export_context_.export_params.convert_orientation ||
usd_export_context_.export_params.convert_scene_units))
{
Michael Kowalski commented 2024-07-12 22:29:37 +02:00
Member
Copy Link

I think this conditional should be

 if (!(usd_export_context_.export_params.convert_orientation ||
       usd_export_context_.export_params.convert_scene_units))

Note the ! is outside the innermost parentheses. Without this change, no transform is applied to root prims if the Root Prim export param is empty. To reproduce, export the default cube to USDA with an empty Root Prim path with Convert Scene Units set to something other than meters. Note that the prims don't have the scaling set.

I think this conditional should be ``` if (!(usd_export_context_.export_params.convert_orientation || usd_export_context_.export_params.convert_scene_units)) ``` Note the `!` is outside the innermost parentheses. Without this change, no transform is applied to root prims if the `Root Prim` export param is empty. To reproduce, export the default cube to USDA with an empty `Root Prim` path with `Convert Scene Units` set to something other than meters. Note that the prims don't have the scaling set.
Charles Wardlaw commented 2024-12-17 00:50:57 +01:00
Author
Member
Copy Link

I moved it outside. I'm not entirely clear on the difference so I'd appreciate more folks checking.

I moved it outside. I'm not entirely clear on the difference so I'd appreciate more folks checking.
CharlesWardlaw marked this conversation as resolved
Michael Kowalski reviewed 2024-07-12 23:01:11 +02:00
source/blender/io/usd/usd.hh Outdated
@ -135,2 +145,3 @@
bool allow_unicode = false;
eUSDSceneUnits convert_scene_units = eUSDSceneUnits::USD_SCENE_UNITS_METERS;
float meters_per_unit = 1.0f;
Michael Kowalski commented 2024-07-12 23:01:11 +02:00
Member
Copy Link

Just a question that occurred to me: is convert_scene_units redundant in this case? I.e., would it be sufficient to just have meters_per_unit set appropriately in io_usd.cc and use that to compute the scale? I know this would change some of the logic elsewhere to determine whether to apply scaling by checking if meters_per_unit is close to 1.0, so there are tradeoffs with either approach.

Just a question that occurred to me: is `convert_scene_units` redundant in this case? I.e., would it be sufficient to just have `meters_per_unit` set appropriately in `io_usd.cc` and use that to compute the scale? I know this would change some of the logic elsewhere to determine whether to apply scaling by checking if `meters_per_unit` is close to 1.0, so there are tradeoffs with either approach.
Charles Wardlaw commented 2024-12-16 17:09:25 +01:00
Author
Member
Copy Link

I'd prefer to leave it as is only because the enum makes it evident what was being done in other places.

I'd prefer to leave it as is only because the enum makes it evident what was being done in other places.
CharlesWardlaw marked this conversation as resolved
Aras Pranckevicius commented 2024-10-08 09:10:00 +02:00
Member
Copy Link

This whole units thing reminded me of #128337 which also attempts to add units dropdown to another I/O type. Maybe it would be good to unify that across various I/O types and use shared code?

This whole units thing reminded me of https://projects.blender.org/blender/blender/pulls/128337 which also attempts to add units dropdown to another I/O type. Maybe it would be good to unify that across various I/O types and use shared code?
Charles Wardlaw added 4 commits 2024-12-16 15:54:52 +01:00 
# Conflicts:
#	source/blender/editors/io/io_usd.cc
#	source/blender/io/usd/usd.hh
Charles Wardlaw added 2 commits 2024-12-17 00:50:38 +01:00
Charles Wardlaw added 1 commit 2024-12-17 01:12:17 +01:00
Charles Wardlaw added 1 commit 2024-12-17 01:13:00 +01:00
Hans Goudey changed title from USD: Adding an option to convert the scene's meters per unit value to USD: Add an option to convert the scene's meters per unit value 2024-12-17 01:26:46 +01:00
Michael Kowalski reviewed 2024-12-18 12:04:39 +01:00
source/blender/editors/io/io_usd.cc
@ -860,0 +898,4 @@
"convert_scene_units",
rna_enum_usd_convert_scene_units_items,
eUSDSceneUnits::USD_SCENE_UNITS_METERS,
"t",
Michael Kowalski commented 2024-12-18 12:04:39 +01:00
Member
Copy Link

It looks like the UI label is a typo. Perhaps "t" should be "Units"?

It looks like the UI label is a typo. Perhaps "t" should be "Units"?
Michael Kowalski requested changes 2024-12-19 06:11:24 +01:00
Michael Kowalski left a comment
Member
Copy Link

This looks good! I had just a couple of comments.

Also, I think for both camera import and camera export we should apply scaling to the near and far clipping planes and the focal distance.

BTW, I'm available to help make these changes if you don't have time.

These are all the comments I have. @deadpin I'd love to get your opinion as well, when you have a chance to look at this.

This looks good! I had just a couple of comments. Also, I think for both camera import and camera export we should apply scaling to the near and far clipping planes and the focal distance. BTW, I'm available to help make these changes if you don't have time. These are all the comments I have. @deadpin I'd love to get your opinion as well, when you have a chance to look at this.
source/blender/editors/io/io_usd.cc
@ -1364,1 +1430,4 @@
namespace blender::io::usd {
double get_scene_scale_from_export_params(const struct USDExportParams *params)
Michael Kowalski commented 2024-12-19 05:59:22 +01:00
Member
Copy Link

I think it might be clearer to name this function get_meters_per_unit() because in the code you use the inverse of the returned value for the scale. But I don't feel strongly about this.

I think it might be clearer to name this function `get_meters_per_unit()` because in the code you use the inverse of the returned value for the scale. But I don't feel strongly about this.
Jesse Yurkovich commented 2024-12-19 06:50:13 +01:00
Member
Copy Link

Additionally, this can be defined inside usd_capi_export probably. No need to have it all the way over in editors/io.

Additionally, this can be defined inside usd_capi_export probably. No need to have it all the way over in editors/io.
source/blender/io/usd/intern/usd_writer_camera.cc
@ -58,6 +58,9 @@ static void camera_sensor_size_for_render(const Camera *camera,
void USDCameraWriter::do_write(HierarchyContext &context)
{
const float unit_scale = float(
Michael Kowalski commented 2024-12-19 06:03:18 +01:00
Member
Copy Link

As per the previous comment about naming this function, maybe unit_scale should be renamed meters_per_unit:

const float meters_per_unit = float (get_meters_per_unit(...));
As per the previous comment about naming this function, maybe `unit_scale` should be renamed `meters_per_unit`: ``` const float meters_per_unit = float (get_meters_per_unit(...)); ```
Jesse Yurkovich commented 2024-12-19 06:51:25 +01:00
Member
Copy Link

You can keep this variable as double precision too.

You can keep this variable as double precision too.
source/blender/io/usd/intern/usd_writer_camera.cc
@ -76,3 +79,3 @@
* = 100 * stage_meters_per_unit
*/
const float tenth_unit_to_mm = 100.0f * scene->unit.scale_length;
const float tenth_unit_to_mm = float(unit_scale * 100.0);
Michael Kowalski commented 2024-12-19 04:39:54 +01:00
Member
Copy Link

Maybe to be compatible with the previous scaling behavior and not break existing scenes we should keep the original scene->unit.scale_length factor as well, i.e.,

const float tenth_unit_to_mm = float(100.0  *  meters_per_unit * scene->unit.scale_length)

(assuming we renamed the unit_scale variable above.

Maybe to be compatible with the previous scaling behavior and not break existing scenes we should keep the original `scene->unit.scale_length` factor as well, i.e., ``` const float tenth_unit_to_mm = float(100.0 * meters_per_unit * scene->unit.scale_length) ``` (assuming we renamed the `unit_scale` variable above.
Jesse Yurkovich reviewed 2024-12-22 22:16:13 +01:00
source/blender/io/usd/intern/usd_capi_import.cc Outdated
@ -292,0 +291,4 @@
const double stage_meters_per_unit = UsdGeomGetStageMetersPerUnit(stage);
data->settings.stage_meters_per_unit = stage_meters_per_unit;
if (data->params.apply_unit_conversion_scale) {
Jesse Yurkovich commented 2024-12-22 22:16:13 +01:00
Member
Copy Link

The code relating to stage_meters_per_unit is now inside the USDStageReader ctor and the adjustment to params.scale can happen inside there now.

The code relating to `stage_meters_per_unit` is now inside the `USDStageReader` ctor and the adjustment to params.scale can happen inside there now.
Michael Kowalski reviewed 2024-12-29 00:09:14 +01:00
source/blender/io/usd/intern/usd_capi_export.cc Outdated
@ -154,6 +155,10 @@ static void ensure_root_prim(pxr::UsdStageRefPtr stage, const USDExportParams &p
return;
}
if (!(params.convert_orientation || params.convert_scene_units)) {
Michael Kowalski commented 2024-12-29 00:09:14 +01:00
Member
Copy Link

This conditional should be removed because it causes the root prim to be an undefined prim when the convert_scene_units and convert_orientation export options are both off.

This conditional should be removed because it causes the root prim to be an undefined prim when the `convert_scene_units` and `convert_orientation` export options are both off.
makowalski marked this conversation as resolved
Michael Kowalski added 3 commits 2024-12-29 00:12:55 +01:00 
Note this commit won't compile due to merge conflicts.
Removed early return logic which was preventing
the root prim from being defined as an Xform when
convert_scene_units and convert_orientation export
options are both off.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feature/usd_convert_to_cm:CharlesWardlaw-feature/usd_convert_to_cm
git checkout CharlesWardlaw-feature/usd_convert_to_cm
Sign in to join this conversation.
Reviewers
No reviewers
Jesse Yurkovich
Michael Kowalski
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
BlendFile

  
Interest
Blender Asset Bundle

  
Interest
Code Documentation
Code comments, Python/RNA API descriptions

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
FBX
FBX I/O related topics

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
USD

  
Interest
UV Editing

  
Interest
Undo

  
Interest
User Interface

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Viewport & EEVEE

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest
glTF
glTF format I/O topics

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, see policy: https://developer.blender.org/docs/handbook/bug_reports/vulnerability_reports/

  
Module
Animation & Rigging

  
Module
Asset System

  
Module
Core

  
Module
Development Management

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Module
Viewport & EEVEE

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
Windows

  
Platform
macOS

  
Severity
High

  
Severity
Low

  
Severity
Normal

  
Severity
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
BlendFile
Interest
Blender Asset Bundle
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
FBX
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
USD
Interest
UV Editing
Interest
Undo
Interest
User Interface
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest
glTF
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Asset System
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
Windows
Platform
macOS
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
USD
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#122804
No description provided.
Delete Branch "CharlesWardlaw/blender:feature/usd_convert_to_cm"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?