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

[for reference] Add Support for Image Planes (as a concrete schema type) #1

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

nxkb
Copy link
Collaborator

@nxkb nxkb commented Sep 25, 2023

Description of Change(s)

This is a full feature reference Pull Request (used in production at Luma) to help guide development of the ultimate Image Plane support in the USD core.
usd-proposal-pr
link to wg draft

Adds ImagePlane prim type, as a textured plane parented to the camera that will display in usdview to support VFX plate workflows.

implementation author: @sirpalee

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@@ -0,0 +1,25 @@
-- glslfx version 0.1
Copy link

Choose a reason for hiding this comment

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

is the introduction of imaging/lib/hdSt/shaders/... intentional?

@@ -12,11 +12,13 @@ pxr_library(usdGeom
usd
work
${Boost_PYTHON_LIBRARY}
${TBB_tbb_LIBRARY}
${OIIO_LIBRARIES}
Copy link

Choose a reason for hiding this comment

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

OpenUSD 23.11 has OpenEXR support built in, so it shouldn't be necessary to directly link OIIO or OpenEXR

Choose a reason for hiding this comment

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

exr does not support 8-16 etc formats

Copy link

Choose a reason for hiding this comment

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

Sure, to be more clear about my comment, we won't be able to add a dependency on OpenImageIO to usdGeom. Loading textures is a function of the Hio library, and Hio already has a plugin to support OpenImageIO reading and writing.

doc = "Rotation value for the image plane. Specified in degrees."
)
int2 coverage = (0, 0) (
doc = "Coverage of the image in pixels. 0 is equal to the image's size."
Copy link

Choose a reason for hiding this comment

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

we typically use coverage to mean geometric coverage of a texel. OpenEXR uses the word window ~ perhaps windowSize and windowOrigin would work here?

Copy link

@revisionfx revisionfx Nov 21, 2023

Choose a reason for hiding this comment

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

I think he means protection (0.05) or padding +0.05 (same as overscan 1.05)

doc = "Alpha Gain"
)
float depth = 100.0 (
doc = "Depth"
Copy link

Choose a reason for hiding this comment

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

Does depth mean distance from the local origin, in scene units? I wonder if the word distance might be the right choice?

doc = "Fitting mode for the image plane."
)
float2 offset = (0.0, 0.0) (
doc = "Image plane offset in inches."
Copy link

Choose a reason for hiding this comment

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

How do imageCenter, and offset relate? How do inches relate to scene units?

doc = "3D offset for the image plane."
)
float2 size = (-1.0, -1.0) (
doc = "Image plane size in inches. Less than zero means equal to the camera aperture size."
Copy link

Choose a reason for hiding this comment

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

if we know the camera parameters, and the distance of the image plane from the camera, wouldn't we want to specify this in NDC? I'm not sure why inches is the choice here.

Copy link

@revisionfx revisionfx Nov 21, 2023

Choose a reason for hiding this comment

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

units should be inherited - imperial or metric. Would call this Physical Dimension,
In practical cameras, sensor are defined in mm, with 4 digits of precision being the tiniest photosite known (0.7 nm on some samsung phone sensor).
Not clear what negative value reference is? Not a fan of semantic associated to sign...

doc = "Using frame extension"
)
int frameOffset = 0 (
doc = "Frame offset"
Copy link

Choose a reason for hiding this comment

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

I realize this is necessary since USD hasn't got support for multiframe textures. I have a feeling this should be addressed upstream. In terms of this proposal, this is probably the right choice, but we should bookmark the problem for further discussion.

doc = "Coverage origin in pixels. Clamped to -image's size and image's size."
)
bool useFrameExtension = false (
doc = "Using frame extension"
Copy link

Choose a reason for hiding this comment

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

What is a frame extension? Perhaps a little more doc.

doc = "Depth"
)
float squeezeCorrection = 1.0 (
doc = "Squeeze Correction"
Copy link

Choose a reason for hiding this comment

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

Might we name this pixelAspectRatio? squeeze is fine, but I'm biased to OpenEXR conventions :)

Choose a reason for hiding this comment

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

-pedantic:

squeeze correction?

theory/convention is you have "Anamorphic Squeeze" = Optical Compression (lens thing, how image is projected on sensor). A camera lens is described as 1.33X, 2X anamorphic... not 0.5, Yes?
As in, to account at lens distortion level.

and PAR- Pixel Aspect Ratio (a sampling thing, what is a pixel?...) which in contemporary cameras is always 1.0 (only legacy video formats, NTSC | PAL had non-square pixels). Yes?
For example a squeezed SBS stereo is not anamorphic, is squeezed when recording video (storage aspect ratio versus display aspect ratio).
And something to deal with I guess for a background image that is not square pixels if you overlay render over.
[e.g: In "scope" the desqueeze is performed by the projector lens- reversing the camera lens anamorphic]

In practice at fileIn level: Anamorphic Stretch = Anamorphic Desqueeze = 1/Anamorphic_Squeeze = Pixel Aspect Ratio = is all the same transform.
There might be a restoration project somewhere in the universe having to deal with scope telecined in an NTSC or PAL context, but not a fan of supporting legacy complexity.

  • the oval shape bokeh "effect" as if it was shot anamorphic (as in some shots in Toy Story 4)
    vs
  • putting a background image on the "film back" (sensor crop dimension) of Maya does have lens squeeze ratio as parameter, I forget if people still have to compensate with overscan so the back plate matches?

I see the expression Anamorphic Desqueeze as most used to describe stretching image to fit the desired image aspect ratio.

Copy link

Choose a reason for hiding this comment

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

I'd be fine with a naming that includes the word "anamorphic" and clarity in the rest of the name around whether one is to multiply or divide in order to undistort an image.

doc = "Height"
)
float alphaGain = 1.0 (
doc = "Alpha Gain"
Copy link

@meshula meshula Oct 28, 2023

Choose a reason for hiding this comment

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

alpha gain could use an explanation.

Choose a reason for hiding this comment

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

+1 - same question, not a thing

doc = "The frame offset for animated textures."
)
token fit = "best" (
allowedTokens = ["fill", "best", "horizontal", "vertical", "to size"]
Copy link

Choose a reason for hiding this comment

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

best and to size should be described. I can't guess what they are without reading the code....

Choose a reason for hiding this comment

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

In ASC-FDL template, best was ignored as an option as it's not the same meaning in different apps... Also what does fill mean?

Copy link

Choose a reason for hiding this comment

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

My understanding from previous discussion is that fill means "stretch the image arbitrarily such that it fills the rectangle".

doc = "Asset path to the file containg the image data."
)
double frame = 0.0 (
doc = "The frame offset for animated textures."
Copy link

@meshula meshula Oct 28, 2023

Choose a reason for hiding this comment

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

frameOffset might be more descriptive

doc = "Precached frame count"
)
float width = 0.0 (
doc = "Width"
Copy link

Choose a reason for hiding this comment

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

width of the image plane in NDC, same question for height?

@@ -0,0 +1,368 @@
//
// Copyright 2016 Pixar
Copy link

@meshula meshula Oct 28, 2023

Choose a reason for hiding this comment

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

year of introduction would be 2023 (wherever a new file is introduced)

@meshula
Copy link

meshula commented Oct 28, 2023

This is looking pretty good. It's pretty obvious, I imagine, from my questions/comments that I'm trying to get a mental picture of the model. I'm wondering if a diagram might be created that shows a camera frustum, the image plane, and how all the values modify the display of the image plane? That would really help me understand all the moving parts :)

@chadrik
Copy link

chadrik commented Nov 21, 2023

I love that this PR is getting some traction right now, but the authors of this PR are no longer at Luma, and nor am I, so I can't speak to whether there are resources at Luma to work on this. I would encourage others to carry this forward and make a proposal to the USD community.

@nxkb
Copy link
Collaborator Author

nxkb commented Nov 22, 2023

You can carry this forward by contributing to the latest proposal or adapting this PR to match that proposal.
Here is a link to the proposal as far as we got: PixarAnimationStudios/OpenUSD-proposals#22

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.

5 participants