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

UsdLux update #2758

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

anderslanglands
Copy link

Description of Change(s)

This is a draft PR that is NOT meant to be merged, intended to provide a focal point for discussion. There are several points in the described behaviour that need agreement from the community.

This PR adds clarifying documentation of the quantities and behaviour of the parameters in UsdLux, all of which are expressed as changes to the doc comments and overview.dox in pxr/usd/usdLux.

It also adds a reference implementation to the hdEmbree example delegate, by modifying that delegate to perform direct lighting according to the specification in the (revised) UsdLux documentation. If no lights are in the stage, hdEmbree falls back to ambient occlusion, as before.

There is a separate repo, here https://github.com/anderslanglands/luxtest there does a bunch of "golden image" unit test of the behaviour and compares renderers. It's all a bit manual at the moment but it generates this result: https://anderslanglands.github.io/luxtest/luxtest.html

Questions for the USD maintainers:

  1. What's the best way of incorporating the changes to UsdLux here. Since it implies a behavioral change on everyone implementing it, it feels like versioning up the schema would be the right approach? If so, what code changes are required to do that exactly?
  2. Are we OK with modifying the Embree delegate in this way, or would it be preferred to split it out into a separate delegate? Or do we want to just lock the new behaviour behind a feature flag/env var?
  3. What sort of testing functionality do we want to include? It might be nice to include some part of the luxtest repo to allow people to run tests of a delegate against the reference easily. Not sure how to go about setting that up.

@meshula
Copy link
Member

meshula commented Oct 26, 2023

Hi Anders, looking forward to reading this more closely.

Could you perhaps submit 51616a6 as a separate PR? That functionality's generally useful and has been requested by others.

@anderslanglands
Copy link
Author

@meshula I already did here: #2674 although weirdly I can't; find that from the PR search. Looking at it now I probably shouldn't have made that against release but against dev. I'll delete that, rebase and resubmit.

@meshula
Copy link
Member

meshula commented Oct 26, 2023

Ok, that's weird! I can find it by manual browsing as well.

@anderslanglands
Copy link
Author

OK I've made a new PR rebased on dev here: #2762

float _peakIntensity = 0;
float _power = 0;

// Types of angle representation in IES files. Currently, only B and C are supported.
Copy link
Member

@meshula meshula Oct 27, 2023

Choose a reason for hiding this comment

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

A to C is

V = Y + 90
L = -X for -180 < X < 0
L = 360-X for 0 < X < 180
L = 0 or L=360 for X = 0

Should be trivial to add by translating to C on load? Type A is common for automative headlights.


// TODO(lukas): Test whether the current type B processing can also deal with
// type A files. In theory the only difference should be orientation which we
// ignore anyways, but with IES you never know...
Copy link
Member

Choose a reason for hiding this comment

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

see my note on ies.h. The formula I posted is from IESNA LM-75-01, so we don't have to guess :)

return false;
}

// Handle the tilt data block
Copy link
Member

Choose a reason for hiding this comment

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

have you scanned past the IESNA:XXXX line somewhere? It's common for the first line of an IES file to be

IESNA:LM-63-2002
IESNA:LM-63-1995
IESNA91
IESNA:LM-63-1991
IESNA:LM-63-1986

If the line isn't there, then the file is by default assumed to be IESNA:LM-63-1986. My IES test corpus commonly had all of the above, but there isn't any impact on the parser here. Mostly, if the IESNA* string appears in the first line it should be skipped.

Choose a reason for hiding this comment

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

This is handled by the constructor of IESTextParser, it skips directly to the first occurrence of \nTILT=.

_intensities.swap(newintensity);
_h_angles.swap(_v_angles);

float h_first = _h_angles[0], h_last = _h_angles[_h_angles.size() - 1];
Copy link
Member

Choose a reason for hiding this comment

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

A common IES file type found in the wild has a single horizontal angle value, resulting in an h_last of 0.f. In this case, the cone angle will be less than 180 degrees. The emissive pattern is radially symmetric and can be constructed trivially. A 1d vector can also be interpolated as an optimization for the shader, but ... exercise for the reader.

Choose a reason for hiding this comment

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

This is currently covered in the Type C handling, but not for B/A, since the standard only mentions the "single horizontal angle" case for Type C.
It wouldn't be hard to add it to B/A, though, and doesn't hurt to have.

@meshula
Copy link
Member

meshula commented Oct 27, 2023

The IES parser in general looks very nice. I added some notes that should enable parsing all the variants I had collected for a test corpus.

@meshula
Copy link
Member

meshula commented Oct 27, 2023

Here's a radially symmetric file for testing

IESNA:LM-63-2002
[TEST] 15905
[TESTLAB] LUMINAIRE TESTING LABORATORY, INC.
[ISSUEDATE] 06-11-2009
[MANUFAC] COLOR KINETICS
[LUMCAT] C-SPLASH 2-FROSTED-ALL ON
[LUMINAIRE] CAST BRASS HOUSING, FROSTED GLASS ENCLOSURE.
[LAMP] 6 RED, 6 GREEN, 6 BLUE LEDS WITH CLEAR PLASTIC OPTICS BELOW EACH.
[LAMPCAT]
[BALLAST] PDS-60 24V POWER SUPPLY WITH COLOR DIAL
[OTHER] ELECTRICAL VALUES: 120.0VAC, 0.3514A, 41.71W
[OTHER] NOTE: THIS TEST WAS PERFORMED USING THE CALIBRATED
[OTHER]       PHOTODETECTOR METHOD OF ABSOLUTE PHOTOMETRY.*
[OTHER] MOUNTING: BRACKET/SURFACE
TILT=NONE
1    515  1.0 361  1 1 1  -0.542  -0.542   0.000
1.0 1   41.7
0.0 0.5 1.0 1.5 2.0 2.5 3.0 3.5 4.0 4.5 5.0 5.5 6.0 6.5 7.0 7.5 8.0 8.5 9.0 9.5
10.0 10.5 11.0 11.5 12.0 12.5 13.0 13.5 14.0 14.5 15.0 15.5 16.0 16.5 17.0
17.5 18.0 18.5 19.0 19.5 20.0 20.5 21.0 21.5 22.0 22.5 23.0 23.5 24.0 24.5
25.0 25.5 26.0 26.5 27.0 27.5 28.0 28.5 29.0 29.5 30.0 30.5 31.0 31.5 32.0
32.5 33.0 33.5 34.0 34.5 35.0 35.5 36.0 36.5 37.0 37.5 38.0 38.5 39.0 39.5
40.0 40.5 41.0 41.5 42.0 42.5 43.0 43.5 44.0 44.5 45.0 45.5 46.0 46.5 47.0
47.5 48.0 48.5 49.0 49.5 50.0 50.5 51.0 51.5 52.0 52.5 53.0 53.5 54.0 54.5
55.0 55.5 56.0 56.5 57.0 57.5 58.0 58.5 59.0 59.5 60.0 60.5 61.0 61.5 62.0
62.5 63.0 63.5 64.0 64.5 65.0 65.5 66.0 66.5 67.0 67.5 68.0 68.5 69.0 69.5
70.0 70.5 71.0 71.5 72.0 72.5 73.0 73.5 74.0 74.5 75.0 75.5 76.0 76.5 77.0
77.5 78.0 78.5 79.0 79.5 80.0 80.5 81.0 81.5 82.0 82.5 83.0 83.5 84.0 84.5
85.0 85.5 86.0 86.5 87.0 87.5 88.0 88.5 89.0 89.5 90.0 90.5 91.0 91.5 92.0
92.5 93.0 93.5 94.0 94.5 95.0 95.5 96.0 96.5 97.0 97.5 98.0 98.5 99.0 99.5
100.0 100.5 101.0 101.5 102.0 102.5 103.0 103.5 104.0 104.5 105.0 105.5 106.0
106.5 107.0 107.5 108.0 108.5 109.0 109.5 110.0 110.5 111.0 111.5 112.0 112.5
113.0 113.5 114.0 114.5 115.0 115.5 116.0 116.5 117.0 117.5 118.0 118.5 119.0
119.5 120.0 120.5 121.0 121.5 122.0 122.5 123.0 123.5 124.0 124.5 125.0 125.5
126.0 126.5 127.0 127.5 128.0 128.5 129.0 129.5 130.0 130.5 131.0 131.5 132.0
132.5 133.0 133.5 134.0 134.5 135.0 135.5 136.0 136.5 137.0 137.5 138.0 138.5
139.0 139.5 140.0 140.5 141.0 141.5 142.0 142.5 143.0 143.5 144.0 144.5 145.0
145.5 146.0 146.5 147.0 147.5 148.0 148.5 149.0 149.5 150.0 150.5 151.0 151.5
152.0 152.5 153.0 153.5 154.0 154.5 155.0 155.5 156.0 156.5 157.0 157.5 158.0
158.5 159.0 159.5 160.0 160.5 161.0 161.5 162.0 162.5 163.0 163.5 164.0 164.5
165.0 165.5 166.0 166.5 167.0 167.5 168.0 168.5 169.0 169.5 170.0 170.5 171.0
171.5 172.0 172.5 173.0 173.5 174.0 174.5 175.0 175.5 176.0 176.5 177.0 177.5
178.0 178.5 179.0 179.5 180.0
0.0
1996 1994 1984 1967 1945 1916 1880 1840 1794 1743 1689 1632 1572 1510 1447 1384
1320 1255 1192 1131 1069 1010 954 900 847 797 749 704 661 620 583 547 513 482
453 425 400 376 354 333 314 296 280 264 250 236 224 212 202 191 182 173 165
157 150 143 137 131 126 120 115 111 106 102 98 95 91 88 85 82 79 76 74 71 69
67 65 63 61 59 57 55 54 52 51 49 48 46 45 44 43 41 40 39 38 37 36 35 34 33 32
31 30 30 29 28 27 26 26 25 24 24 23 22 22 21 20 20 19 19 18 18 17 17 16 15 15
15 14 13 13 13 12 12 11 11 10 10 9 9 9 8 8 7 7 7 6 6 5 5 5 4 4 3 3 3 2 2 2 1 1
1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0

@anderslanglands
Copy link
Author

anderslanglands commented Oct 27, 2023 via email

@sunyab
Copy link
Contributor

sunyab commented Oct 30, 2023

Filed as internal issue #USD-8880

@@ -62,6 +62,38 @@ class SdfAssetPath;
/// regardless of whether LightAPI is included as a built-in API of the prim
/// type (e.g. RectLight or DistantLight) or is applied directly to a Gprim
/// that should be treated as a light.
///
/// <b>Quantities and Units</b>

Choose a reason for hiding this comment

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

I could be wrong about this, but for the usdLux header comment changes, I believe you might need to update these first in usdLux's schema.usda and then use usdGenSchema.py to generate the headers with the updated doxygen comments. See for example https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/usdLux/schema.usda#L49-L54

@anderslanglands
Copy link
Author

anderslanglands commented Nov 8, 2023 via email

anderslanglands and others added 5 commits November 21, 2023 15:45
adds a command-line option, --disableDefaultight to usdrecord that causes UsdAppUtilsFrameRecorder to not add the default camera light to the imaging engine.
This modifies the hdEmbree example plugin to do direct lighting so as to
provide a reference implementation of the expected behaviour for UsdLux.

If no lights are present in the stage, the old ambient occlusion path
will be used.
Adds explanation of expected behviour to UsdLux docs.
@lukasstockner
Copy link

Hi, author of the Cycles IES parser here!

You might want to have a look at https://projects.blender.org/blender/blender/pulls/114689 - the topic of handling Type A files also came up on our end a few weeks ago, so I've updated the parser to be able to handle them.
As part of that, I've also removed the angle checks in the processing code - while the IES spec is explicit about which specific angle ranges are allowed, I've found many files deviate from that, so Cycles now doesn't care anymore and just tries to make the best out of any input.

@spiffmon
Copy link
Member

@anderslanglands - apologies, you can revert the change to usdrecord . We made a similar change (thanks to a patch from @creijon) on Feb 8th that is just hung up on getting pushed to gitHub due to the pending 24.03 release.

@anderslanglands
Copy link
Author

@anderslanglands - apologies, you can revert the change to usdrecord . We made a similar change (thanks to a patch from @creijon) on Feb 8th that is just hung up on getting pushed to gitHub due to the pending 24.03 release.

You mean the change to add a disableDefaultLight option?

@spiffmon
Copy link
Member

spiffmon commented Feb 15, 2024 via email

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.

None yet

6 participants