forked from PixarAnimationStudios/OpenUSD
-
Notifications
You must be signed in to change notification settings - Fork 0
[usdLux] normalized lights divide by surfaceArea expressed in meters^2 #5
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
Closed
pmolodo
wants to merge
270
commits into
pr/usd-lux-update-doc
from
pr/normalize-lights-using-fixed-meters-squared
Closed
[usdLux] normalized lights divide by surfaceArea expressed in meters^2 #5
pmolodo
wants to merge
270
commits into
pr/usd-lux-update-doc
from
pr/normalize-lights-using-fixed-meters-squared
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…GetNodeDiscoveryResults and GetShaderProperties in favor of GetDiscoveryResults and GetProperties, respectively (Internal change: 2356231)
(Internal change: 2356232)
…g VFX2023. I lean towards the reported error (array bounds) being a compiler bug for a few reasons. 1) It appeared after upgrading to VFX 2023. 2) As far as I can tell, the error only happens for the first for-loop over highlight modes and not the second, even though they use the same for-loop conditions, array indices, and syntax. 3) I haven't been able to recreate the error in the compiler explorer (although it doesn't currently support the exact setup from what I can tell i.e. gcc 11.2.1/vfx2023). Simple workarounds like unrolling for-loops or replacing the problematic for-loop entirely with hard-coded statements also fixed the issue. I'm guessing the size_t typing is allowing the compiler to handle bounds checking somewhat differently and avoid the spurious error. One theory is that since the first for-loop is setting all the elements to the same value unconditionally while the second is not, the compiler might be optimizing the first for-loop into a memset (__builtin_memset) that isn't working correctly with the 32-bit index values. (Internal change: 2356294)
CMake 3.30 removed support for FindBoost, so we switched to using the BoostConfig.cmake file. This change is backwards compatible down to Boost 1.70 and much older CMake versions. Apply the following changes: - extraArgs.append('-DBoost_NO_BOOST_CMAKE=OFF') - extraArgs.append('-DBoost_NO_SYSTEM_PATHS=ON') Closes PixarAnimationStudios#3485 (Internal change: 2356335)
Now that we have moved to boost version (1.76+) which has stabilize boost cmake config workflow, we can safely take out Boost_NO_BOOST_CMAKE from OpenUSD build configurations. This is a followup change for 2356335 (Internal change: 2356337)
(Internal change: 2356388)
…omment. (Internal change: 2356389)
Fixes failure to compile with GCC 15 Closes PixarAnimationStudios#3487 (Internal change: 2356412)
Baking should be backing (I believe). Can be confusing as baking is used when talking about pre-rendered shadows. Closes PixarAnimationStudios#3465 (Internal change: 2356415)
….0 prim adapters. (Internal change: 2356416)
…teSpec. Fixes PixarAnimationStudios#3034 Closes PixarAnimationStudios#3319 (Internal change: 2356425)
- swap out usages of `find_executable` with `shutil.which` - replace strtobool with an equivalent function (https://docs.python.org/3.9/distutils/apiref.html#distutils.util.strtobool) Fixes PixarAnimationStudios#3389 Closes PixarAnimationStudios#3417 (Internal change: 2356466)
We are using HdCreateTypedRetainedDataSource() to convert the config VtDictionary to a Container Data Source. This function does not handle nested attributes, so mtlx:version was not making it into Hydra. This change flattens the attributes so that the mtlx:version does make it to Hydra. (Internal change: 2356523)
- Update Alembic Plugin docs as PXR_ENABLE_HDF5_SUPPORT currently defaults to OFF/FALSE. - Remove ASCII Parser Editing/Validation as the text format parser is now PEGTL and FLEX/BISON are no longer in use. (Internal change: 2356566)
(Internal change: 2356655)
…ns of implicit behaviors for SdrShaderProperty types (Internal change: 2356661)
GLSL version string was converted to floating point using sscanf. But sscanf expects decimal separator to match with current locale, while GLSL version string always uses period as decimal separator. This caused Storm to fail e.g. on german systems. Closes PixarAnimationStudios#3467 (Internal change: 2356671)
(Internal change: 2356673)
(Internal change: 2356678)
(Internal change: 2356682)
(Internal change: 2356683)
(Internal change: 2356684)
(Internal change: 2356689)
(Internal change: 2356697) (Internal change: 2356733)
Copying binary data in text mode can cause problems on some locales, so specify binary mode. Closes PixarAnimationStudios#3500 (Internal change: 2356741)
Various fixes related to using the pick task with Vulkan including: - Fixed validation issues around pick task for Vulkan backend - Fixed bad semantic check for depth stencil and depth tokens - Fixed bad aspect mask usage for depth vs depth stencil buffer Closes PixarAnimationStudios#3507 (Internal change: 2356742)
the prim index in USD mode, causing it to ignore instancing restrictions among other things. The underlying issue was that PcpCache::GetPrimIndexInputs wasn't adding the USD mode flag to the returned inputs. PcpPrimIndex-internal uses of of GetPrimIndexInputs were adding the USD mode flag to them manually but the external uses of it (namely ComputeExpandedPrimIndex) did not know to do this. In the end I refactored the GetPrimIndexInputs function to return a const ref result that always includes the USD mode flag and made clients that want to change the inputs have to explicitly make a copy of the inputs. I also wrapped PcpPrimIndex::IsUsd to python for easy testing. Fixes PixarAnimationStudios#3526 (Internal change: 2356795)
Missed adding the typename to this data source. Without it the typeName was getting lost when rendering with HdPrman. This change also updates hdMtlx to only ask the network interface for the param data on node parameters and not the colorSpace or typeName parameters. (Internal change: 2356897)
…ad of the material network HdPrman currently looks to the nodeInterface to find the nodeType, however, if the nodedef name has changed between versions of MaterialX and we relied on MaterialX's document upgrade process used in HdMtlx, we would not get the correct nodeType. This change instead looks to the MaterialX document for the nodeType. Since the document will already have been upgraded at this point, it wil contain the correct updated nodedef/nodeType name. (Internal change: 2356901)
(Internal change: 2356953)
scene globals schema. HdsiSceneGlobalsSceneIndex also gains a way to set these, as a way to allow app state to drive them. We note that a camera prim path does also already exist within the render product schema, as reflected from USD's UsdRenderSettingsBase schemas. That is intended to support rendering systems that can render multiple views simultaneously, although in common practice there is a single shared camera. Renderers being driven by the render product schema should continue to honor it. The scene-global camera path is intended to nominate a primary camera for use in scene generation (such as providing consistent frustum-pruning or tesselation heuristcs across render products). It also serves to provide a camera in cases where no render settings prim was provided. We expect most filtering scene indexes that need to query a "current" camera, upstream of the renderer, to use this. Motion blur scene indexes now consult scene globals for timeCodesPerSecond (informally, FPS). Since the application has not had a chance to configure the scene globals at scene index construction time, these scene indexes defer retrieving the timeCodesPerSecond value until later. (Internal change: 2362252)
…ormsamples Karma treats these as explicit values, whereas we have been treating them as minimums. This fix switches us to treating them as explicit values. (Internal change: 2362258)
These tests were missing arguments that were used for the baseline images. This was not caught earlier because a bug in the idiff tool from the OpenImageIO version used internally caused it to incorrectly report that there were no diffs. Switching to OIIO v2.5.6.0 (from change 2349320) exposed this problem. (Internal change: 2362293)
This PR adds an Accessibility API schemas to UsdUI as an attempt to make it a Universally Accessible Scene Description. A brief overview of the schema: [Proposal](https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main/proposals/accessibility) ``` def "Foo"( apiSchemas = ["AccessibilityAPI:default"] ) { string accessibility:default:label = "Short Label" string accessibility:default:description = "A longer description of this prim" uniform token accessibility:default:priority = "standard" } ``` UsdUIAccessibilityAPI is a multiple apply schema. Each instance has three attributes: label: A short description of the prim description: An extended description of the prim priority: A hint to a runtime on how this instance should be valued There are a few small changes from the PR as originally filed: - Docs in schema.usda have been line-wrapped so that the generated code adheres to the 80 character line limit and updated to correct a few typos. - testUsdUIAcessibilityAPI was renamed to correct the typo. Closes PixarAnimationStudios#3271 (Internal change: 2362308)
If a resource registry has a static lifetime object as its root owner, then we can encounter a static lifetime ordering issue, since this registry map also has a static lifetime. It's possible that due to the unspecified ordering of static destruction, this map is destroyed before a registry, in which case calling _Unregister() would be undefined behaviour. To prevent this we use a weak ownership, and skip the call when the map is dead because it no longer matters. Closes PixarAnimationStudios#3557 (Internal change: 2362312)
HdPrman should use HdsiVelocityMotionResolvingSceneIndex to resolve velocity-based motion. This lets us remove the redundant and in some cases diverging implementation of velocity motion from hdPrman's motion blur handling and centralizes velocity motion resolution. Since HdsiVelocityMotionResolvingSceneIndex correctly freezes instance scales, adopting it in hdPrman resolves the issue. Note that there are some differences between legacy sceneDelegate-backed scenes and fully sceneIndex-backed scenes. Tests are added to exercise these differences. The changes to hdPrman's motion blur handling are intended to preserve the legacy behavior as best as possible. The divergences arise in situations where time samples have been sparsely authored and can be mitigated by authoring the missing time samples. (Internal change: 2362318) (Internal change: 2362348)
…r hydra 2.0)). (Internal change: 2362322)
(Internal change: 2362325)
function just does what the python wrapping for Pcp.PrimIndex.primStack already does but now accessible from C++. this will used in namespace editing to determine if a resynced prim after a namespace edit has effectively changed. (Internal change: 2362338)
- Use it to implement excludePaths based pruning in UsdImagingGLEngine. (Internal change: 2362343) (Internal change: 2362359)
vkGetPhysicalDevice[Properties/Features] when we already use the "2" version. Also we don't seem to be using VkPhysicalDeviceDescriptorIndexingFeaturesEXT features, so its use was removed. (Internal change: 2362344)
…y setting env var so that tests always pass. Adding missing test case to CMakeLists.txt. Referencing relevant bugs. (Internal change: 2362347) (Internal change: 2362376) (Internal change: 2362382)
…ClipSets - In this simple implementation, we get the previous time sample by getting all time samples from the clipSet and then finding the previous time sample. Note that this is very similar to sdf/data or usd/crateData implementation. - Updated testUsdValueClips to check for preTime values for scenarios where its expected that preTime values will be different by querying a Usd.TimeCode.PreTime(time). NOTE: The current implementation is known to have floating point precision issues at value clips jump discontinuities. A subsequent change will fix/clean this up. All tests catering to querying for pre-time values at jump discontinues are commented out to avoid spurious failures because of floating point precision issues and will be cleaned when the code for this is fixed. (Internal change: 2362350)
(Internal change: 2362358)
…n order to consider layer stacks which are being unmuted or inserted when searching for the node of a changed spec. This prevents some added inert prims from ending up in PcpCacheChanges::didChangePrims and causing USD to treat them as resyncs. (Internal change: 2362362)
(Internal change: 2362371)
(Internal change: 2362373)
…ptr will be returned in the event that the resolved path is a directory. (Internal change: 2362381)
(Internal change: 2362383)
provide UsdStage's ObjectChanged notice with more information about what overall edits have occurred to UsdPrims. This can be queried in the sent notice via the function GetPrimResyncType which allows the client to determine if the reported resync paths are effective renames and/or reparents or even composition no-ops due to downstream dependency fixups. We determine this by having the UsdNamespaceEditor open a special change block on all its dependent stages with the edits stage level prim moves it expected to perform. The stage's change process account for these expected moves and checks that the move results in a prim with the exact same prim stack as before. If it does, that move is included in the ObjectsChanged notice the stage sends so that it can be queried via GetPrimResyncType to allow clients to decide if they can treat the resync in special (and likely more efficient) manner than they could without this information. (Internal change: 2362384)
(Internal change: 2362387)
Updating UsdImagingGLEngine to use it instead of the Hydra 1.0 HdxTaskController if the env var USDIMAGINGGL_ENGINE_ENABLE_TASK_SCENE_INDEX is set. (Internal change: 2362391)
(Internal change: 2362394)
(also added user doc for recently added AccessibilityAPI) (Internal change: 2362395)
(Internal change: 2362396)
8d780db
to
90bfc67
Compare
This allows normalized lights to compose correctly into stages with a different metersPerUnit setting, at a backwards-compatibility cost of a change in luminance of normalized lights before this, in layers with metersPerUnit != 1
90bfc67
to
581b73f
Compare
Closing in favor of: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Intro + Background
Neither the current UsdLux implementation, or the current version of the UsdLux clarifications and changes in this OpenUSD proposal (md) (pdf), explicitly specify how the behavior of
LightAPI.inputs:normalize
relates to linear stage units (metersPerUnit
), and what happens when the linear stage units change (ie, by being composed into another stage with differentmetersPerUnit
). We wish to further update the UsdLux clarification to address this gap.Terms
lsu = linear stage unit(s)
metersPerUnit
- ie,metersPerUnit=.01
implies "the lsu is cm"nit
cd/m^2
)Common usage of the normalize attribute
In theory, you might desire to have normalized lights and non-normalized lights behave differently when composed into a scene with different units... but I struggled to come up with a credible common scenario for that.
Most commonly, normalize is used to allow lighting artists to adjust the size of a light without affecting it's overall illumination output on the surrounding scene. ie, you might want to make shadows softer without dramatically altering the overall light level of the scene. It's used as a way to alter the way you sculpt the light while designing it - not as a way to mark it as a fundamentally different type of light, that should behave in a fundamentally different way from other lights, when placed into a new setting, or when automated, non-artistic operations are done on it.
As transforms associated with changes in scene scale are "non-artistic" (and ideally, automated) changes, it follows that artists will generally want normalized and non-normalized lights to behave similarly in this situation. That is, if they have two lights that appear identical in two_lights.usd, except one is normalized and one isn't, they will want the two lights to stay identical to each other when placed into a new scene.
Assumptions and Observations
Assumption 1: Light dimensions must be in lsu
If this were not the case, then a layer with both lights + geometry would not retain the same relative size when composed into a scene with a different
metersPerUnit
- which is unlikely to be the wanted behavior.Assumption 2: For non-normalized lights,
inputs:intensity
represents luminance innits
This document is intended as a further exploration to the proposed UsdLux clarifications and changes from this OpenUSD proposal (md) (pdf), and that proposal assumes (non-normalized)
inputs:intensity
is in a fixed units,nits
.Observation 1: Direct light sampling for non-normalized, non-ies lights are generally not affected by
metersPerUnit
For non-normalized lights, we've assumed
inputs:intensity
is always a measure of base luminance innits
. Because measurement of luminance is unaffected by distance, it is thus unaffected by a scale change. Most other UsdLux light properties are modifiers to the luminance, whose formulas do not depend on any values which are in scene units. (One possible exception are lights with attached ies files, whose measurements are defined to be luminous intensity values expressed in candela. However, we defer discussion of unit interaction with ies files for another disucssion.) Therefore, direct light sampling for non-normalized, non-ies lights are unaffected by scale changes.By "direct light sampling", I mean the sampling of a light for a ray known to intersect the light. The light dimensions are in lsu, so a
metersPerUnit
change can/will affect their size and position in the scene, and therefore can affect whether a given ray will intersect it, or the exact sampling pattern used. But, once it's known a ray intersects a non-normalized, non-ies light, themetersPerUnit
setting is unimportant.Therefore, other than ensuring the size scale of a non-normalized light is as desired, they will generally not present any special problems when undergoing unit changes.
Normalized Area Light Option 1: Normalized lights divide by scene-relative surface area
Summary of behavior
sizeFactor
for normalized area lights (whereluminance = inputs:intensity / sizeFactor
)is in scene-relative units of
lsu^2
metersPerUnit
into account
When rendering the composed stage, renderers need to:
metersPerUnit
settingWhen composing lighting scenes with different
metersPerUnit
, artists / end users need to:metersPerUnit
inputs:intensity
is in lsu-relative units, and so remains unchangednits
) of the light changesBackwards compatibility behavior
Normalized Area Light Option 2: Normalized lights divide by fixed-unit surface area (meters^2)
Summary of behavior
sizeFactor
for normalized area lights (whereluminance = inputs:intensity / sizeFactor
)is in fixed units of
meters^2
When rendering the composed stage, renderers need to:
metersPerUnit
^2metersPerUnit
^2, we would get an area in units of m^2metersPerUnit
metersPerUnit
, we would get a length in units of mmetersPerUnit
When composing lighting scenes with different
metersPerUnit
, artists / end users need to:metersPerUnit
metersPerUnit
, at render time (see above)Backwards compatibility behavior
metersPerUnit=1
, will be significantly brighter or darker in "old" vs "new" UsdLuxnewLuminance = oldLuminance / metersPerUnit^2
metersPerUnit=.01
), normalized lights in "new" usdLux will have 10,000x higher luminance than with "old" UsdLuxSummary and Analysis
Summary of differences
Here's a summary of differences, with my subjective rating of which option has the superior behavior in each area:
metersPerUnit
Analysis
Big win for Option 2 (Normalized Fixed Units): no manual overrides on lights to preserve correct real-world units
Big win for Option 1 (Normalized Scene-relative units): better backwards compatibility of normalized light intensity for "most" scenes
metersPerUnit=1
), the brightness of normalized lights under Option 2 will change by orders of magnitudemetersPerUnit=1
), it is assumed that "many" / "most" scenes will have their normalized lights change significantly under Option 2Minor win for Option 1 (Normalized Scene-relative units): renderers need no additional unit-aware changes
Conclusion
We have a choice between backwards compatibility (Option 1) vs better / more correct behavior going forward (Option 2)
The UsdLux clarification project is about providing better behavior going forward, at the cost of some backward compatibility, so overall we feel Option 2 is the better choice long-term.