Add a tutorial teaching about lighting in Drake #22631
Conversation
There was a problem hiding this comment.
+@rcory +@jwnimmer-tri for review
General notes:
- Revision 1 and 2 are separate PRs (changes to VTK) that the tutorial depends on. (They are Correct VTK's image-based lighting diffuse calculation #22629 and Preserve direct light specular reflections for smooth materials #22632, respectively). Feel free to ignore them.
- Feel free to tag anything that seems unclear, unhelpful, or any questions that you still have after attempting to read through the tutorial.
@jwnimmer-tri In addition to content, this is also a good time to talk about side files. Now that we have something concrete.
If you haven't reviewed tutorials before, it's very easy. Simply check out the PR and then run:
bazel run //tutorials:configuring_lighting
Reviewable status: LGTM missing from assignees rcory,jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
|
FYI @rcory I'm waiting to let you take a first pass over this, before I do my review. |
SeanCurtis-TRI
force-pushed
the
PR_lighting_tutorial
branch
from
680b0d3 to
bb28cee
Compare
|
I guess Rick is pretty busy. I'll take a look at this tomorrow. |
There was a problem hiding this comment.
Sorry, we were pushing on a plan for data-collect (completed yesterday). I am planning on spending time on this today.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/configuring_lighting.ipynb line 701 at r3 (raw file):
" directed cone limits the directions that rays are emitted. The cone has its\n", " apex at the light `position` and the cone is rotated so its central axis\n", " is aligned with the light's `direction` vector (such the cone's radius\n",
I know what you mean, but this wording threw me off at first. The illustration shows multiple vectors originating from the spot light's center, so when you say "its central axis is aligned with the light's direction", it might be clearer to to say "its central axis is aligned with the light's central direction vector".
tutorials/configuring_lighting.ipynb line 701 at r3 (raw file):
" directed cone limits the directions that rays are emitted. The cone has its\n", " apex at the light `position` and the cone is rotated so its central axis\n", " is aligned with the light's `direction` vector (such the cone's radius\n",
Suggestion:
such that the cone's radius
tutorials/configuring_lighting.ipynb line 706 at r3 (raw file):
" the cone's span, or the angle between the cone's central axis and the\n", " boundary of the cone.\n", " - __Directional light__: This can be thought of a point light that is infinitely\n",
Suggestion:
be thought of as a point
tutorials/configuring_lighting.ipynb line 717 at r3 (raw file):
"\n", "Light interacts with matter in various ways. While the physics is fascinating\n", "and intricate, and rendering technology typically focuses on the _effects_ and\n",
Suggestion:
, rendering technology
There was a problem hiding this comment.
Reviewable status: 30 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/configuring_lighting.ipynb line 48 at r4 (raw file):
"physics (in comparison with older, classical illumination models).\n", "While it is truer to reality, it is still an approximation. There\n", "will inevitably be differences between renderings and reality.\n",
BTW I believe there might also be performance trade-offs (i.e., PBR would be slower)?
tutorials/configuring_lighting.ipynb line 52 at r4 (raw file):
"### What makes it physical?\n", "\n", "PBR has three driving principles:\n",
nit while I'm sure these principles are important, I'm not sure they add much value to this tutorial? Should I care about these details, or would pointing to a Wikipedia article on PBR (for those interesting in learning more) be sufficient?
Edit: I see that you've linked to the Filament PBR documentation below - which is probably sufficient.
tutorials/configuring_lighting.ipynb line 61 at r4 (raw file):
" surfaces.\n", "\n", "PBR provides concrete benefits:\n",
FYI These points are easy to digest and tell me why I should care about PBR.
tutorials/configuring_lighting.ipynb line 93 at r4 (raw file):
" - Set the `force_to_pbr` flag to `true`.\n", " - Add an [environment map](#environment-maps).\n", " - Add a glTF file (Drake's preferred visual geometry representation)."
BTW to clarify, do you mean so long as a single glTF file is referenced in a model, that is sufficient to trigger PBR?
Also, what if someone uses a single glTF as a (Convex) collision geometry, but not otherwise referenced in any illustration roles? I presume PBR would only trigger if illustration (or perception roles?) use glTF?
tutorials/configuring_lighting.ipynb line 584 at r4 (raw file):
"\n", "For subsequent renderings, we may not do all of these (for example, several\n", "renderings in a can use the same geometry)."
Suggestion:
can use the same geometry
tutorials/configuring_lighting.ipynb line 637 at r4 (raw file):
"**ATTENTION**: Expect the rendered image to deviate from what is shown in\n", "the meshcat browser. Meshcat introduces an independent light set from those\n", "defined in the `RenderEngine`. Meshcat also uses a PBR illumination model\n",
Suggestion:
Meshcat also uses a PBR illumination model,
tutorials/configuring_lighting.ipynb line 848 at r4 (raw file):
"the full ground box. We see a circular pool of light. The light is circular\n", "because the spot light's cone of light intersects the ground perpendicularly.\n", "If the spot light were tilted, it the pool of light would be elliptical.\n",
Suggestion:
the pool of light
tutorials/configuring_lighting.ipynb line 853 at r4 (raw file):
"A more subtle difference is between the point light and directional light\n", "images. In both images, the ball's appearance is essentially the same. However,\n", "the ground box is different. With the point light, the ground direclty under\n",
Suggestion:
directly
tutorials/configuring_lighting.ipynb line 853 at r4 (raw file):
"A more subtle difference is between the point light and directional light\n", "images. In both images, the ball's appearance is essentially the same. However,\n", "the ground box is different. With the point light, the ground direclty under\n",
nit I suppose it's really the ground proximal to the ball? (Directly under would be a dark shadow?)
Code quote:
the ground direclty under
tutorials/configuring_lighting.ipynb line 858 at r4 (raw file):
"with the directional light. It's also worth pointing out that _within_ the\n", "pool of light, the spot light's pattern of illumination matches the point\n", "light. Where does this difference come from?\n",
nit This wording is confusing. It seems to point out that the spot and point lights match (within the spot light's pool of light), but then asks "where does this difference come from?". What difference are you referring to? Maybe you are referring to the difference between the point light and directional light, which you forgot to mention here?
Code quote:
"pool of light, the spot light's pattern of illumination matches the point\n",
"light. Where does this difference come from?\n",
tutorials/configuring_lighting.ipynb line 879 at r4 (raw file):
"So, how does that apply to the images above?\n", "\n", "[TODO: illustration]\n",
FYI Not sure if this TODO was left intentionally to be visible in the tutorial? Same for others below.
tutorials/configuring_lighting.ipynb line 976 at r4 (raw file):
" \n", "The inverse-squared law is satisified by c = 0, l = 0, q = 1. In practice,\n", "it sometimes becomes convenient to introduce have the intensity decay\n",
Suggestion:
convenient to have the intensity decay
tutorials/configuring_lighting.ipynb line 977 at r4 (raw file):
"The inverse-squared law is satisified by c = 0, l = 0, q = 1. In practice,\n", "it sometimes becomes convenient to introduce have the intensity decay\n", "more slowly by reintroducing constant or linear factors. The three\n",
Suggestion:
The sum of the three
tutorials/configuring_lighting.ipynb line 980 at r4 (raw file):
"coefficients should total one.\n", "\n", "**Note**: attenuation does not apply to directional lights as there is\n",
Suggestion:
**Note**: Attenuation
tutorials/configuring_lighting.ipynb line 1000 at r4 (raw file):
"\n", "comparator.set_render_engines([\n", " RenderEngineSpec(\"Constant\", make_config(attenuation=[1, 0, 0])),\n",
nit Might be worth mentioning that zero attenuation causes full saturation (which is perhaps the same as having infinite intensity?).
tutorials/configuring_lighting.ipynb line 1051 at r4 (raw file):
"outputs": [], "source": [ "# Add several arbitrary light; each with an arbitrarily different color do better\n",
Suggestion:
lights
tutorials/configuring_lighting.ipynb line 1051 at r4 (raw file):
"outputs": [], "source": [ "# Add several arbitrary light; each with an arbitrarily different color do better\n",
Suggestion:
different color to better
tutorials/configuring_lighting.ipynb line 1124 at r4 (raw file):
"surfaces that would otherwise have been lit. In rendering, we don't get\n", "occlusion for free. We create shadows through a variety of approximations. The\n", "spot light's light cone is our first example. In the real world, a spot light's\n",
Suggestion:
spot light's cone
tutorials/configuring_lighting.ipynb line 1212 at r4 (raw file):
"### What lights can cast shadows?\n", "\n", "Spot lights and directional lights can cast shadows. Point lights cannot.\n",
But why not?? I found myself wondering why the shadow itself for point and spot lights (in this example) wouldn't end up being the same?
Edit: I see that this finally gets answered in the "Spot light limitations" section. Might be worth referring the reader to that section here.
Code quote:
Spot lights and directional lights can cast shadows. Point lights cannot
tutorials/configuring_lighting.ipynb line 1285 at r4 (raw file):
"\n", "- The shadow cast from the directional light is smaller than that of the spot\n", " light Not only that, it even appears smaller than the ball.\n",
Suggestion:
light. Not only that
tutorials/configuring_lighting.ipynb line 1286 at r4 (raw file):
"- The shadow cast from the directional light is smaller than that of the spot\n", " light Not only that, it even appears smaller than the ball.\n", " - The directional shadow *is* definitely smaller than the point light shadow.\n",
Suggestion:
smaller than the spot light shadow
tutorials/configuring_lighting.ipynb line 1638 at r4 (raw file):
"things closer. When the ball is closer to the camera than its shadow, it will\n", "appear larger. When the shadow is closer to the camera than the ball, the\n", "shadow will appear bigger. THe important thing to note is that these shadows are\n",
Suggestion:
The important thing
tutorials/configuring_lighting.ipynb line 1657 at r4 (raw file):
"light properties doesn't have the effect you expect.\n", "\n", "Again, this discussion is going bias heavily towards intuition and\n",
Suggestion:
this discussion is going to bias heavily
tutorials/configuring_lighting.ipynb line 1683 at r4 (raw file):
"direction _l_. The three views _v₀_, _v₁_, and _v₂_ form half-angle\n", "vectors _h₀_, _h₁_, and _h₂_, respectively. Only _h₁_ aligns with\n", "_n_, so only view _v₀_ receives the specular reflection from the\n",
Suggestion:
so only view _v₁_ receives the specular reflection
tutorials/configuring_lighting.ipynb line 1686 at r4 (raw file):
"light source.\n", "\n", "In the real world, surface reflectivity can vary widely some crisp\n",
Suggestion:
can vary widely; some crisp
tutorials/configuring_lighting.ipynb line 1813 at r4 (raw file):
"surface (its normal direction), _and_ the direction from the surface to\n", "the camera.\n", "4. When like strikes a surface at a \"grazing\" angle, the surface exhbits\n",
Suggestion:
When light strikes a surface
tutorials/configuring_lighting.ipynb line 1863 at r4 (raw file):
"reflections, the effect is typically more noticable on dielectric\n", "materials, because their _base_ reflectivity is generally much lower\n", "than of metals.\n",
Suggestion:
than that of metals
tutorials/configuring_lighting.ipynb line 1951 at r4 (raw file):
"across the two spheres. The metallic sphere's \"hot spot\" is\n", "larger and somewhat oversaturated in the center (we'll deal\n", "with the oversaturatin when we discuss [exposure](#exposure)\n",
Suggestion:
oversaturation
There was a problem hiding this comment.
Reviewable status: 38 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/configuring_lighting.ipynb line 1987 at r4 (raw file):
"We'll start with a simple environment map and a mirror ball.\n", "\n", "The image below shows a ball in a simple environment. The mirror ball sites on\n",
Suggestion:
The mirror ball sits on
tutorials/configuring_lighting.ipynb line 1992 at r4 (raw file):
"\n", "\n", "**Figure:** the physical environment.\n",
Suggestion:
The physical environment
tutorials/configuring_lighting.ipynb line 2007 at r4 (raw file):
"\n", "\n", "**Figure:** the environment map drawn from the physical scene shown above."
Suggestion:
The environment map drawn
tutorials/configuring_lighting.ipynb line 2224 at r4 (raw file):
"such specular reflection.\n", "\n", "But this image illustrates to additional phenomena: Fresnel\n",
Suggestion:
this image illustrates two additional phenomen
tutorials/configuring_lighting.ipynb line 2235 at r4 (raw file):
"so we observe a more intense reflection.\n", "\n", "Both spheres have a reddish tint on that covers a large portion\n",
Suggestion:
have a reddish tint that covers
tutorials/configuring_lighting.ipynb line 2241 at r4 (raw file):
"map's contribution to the diffuse lighting effects.\n", "\n", "Remember, diffuse illumination when light strikes a surface,\n",
nit The wording in this sentence seems odd.
tutorials/configuring_lighting.ipynb line 2248 at r4 (raw file):
"it must be included in the diffuse calculation.\n", "\n", "In this case, for any givn point on the sphere, fully half of\n",
Suggestion:
for any given point
tutorials/configuring_lighting.ipynb line 2248 at r4 (raw file):
"it must be included in the diffuse calculation.\n", "\n", "In this case, for any givn point on the sphere, fully half of\n",
nit not quite sure what "fully half" means.
There was a problem hiding this comment.
Reviewable status: 46 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
FYI when I start the tutorial, all cells in the notebook show up as already evaluated (and all renderings already shown without running any code). I'm assuming this isn't intentional (since you want the reader to start fresh).
I can easily clear things by going to the top menu and selecting Cell>All Output>Clear and saving the file.
tutorials/configuring_lighting.ipynb line 1117 at r4 (raw file):
"source": [ "<a id=\"shadows\"></a>\n", "## Shadows\n",
BTW in the discussion of shadows below, the topic of shadow edge sharpness was never discussed. I don't believe this is yet supported (or exposed?) in Drake, but probably worth mentioning at some point.
tutorials/configuring_lighting.ipynb line 2244 at r4 (raw file):
"some portion gets refracted _into_ the material, bounces around\n", "and, finally, some remaining portion is emitted again. The\n", "_total_ diffuse affect is the sum of all such light. If\n",
Suggestion:
effect
tutorials/configuring_lighting.ipynb line 2314 at r4 (raw file):
" intensity light sources manifesting as dark colors to film and sensors. As\n", " counter-intuitive as it might seem, a pixel with normalized color\n", " (0.01, 0.01, 0.01) image is putting out 1 W of light, just like a white\n",
Suggestion:
(0.01, 0.01, 0.01) is putting
tutorials/configuring_lighting.ipynb line 2335 at r4 (raw file):
"\n", " - PNG: a normalized image with brightness of 1 W at each pixel.\n", " - 7.5 HDR: an HDR image with maximum brightness of 7.5 W.\n",
Suggestion:
brightness of 7.5 W at each pixel
tutorials/configuring_lighting.ipynb line 2336 at r4 (raw file):
" - PNG: a normalized image with brightness of 1 W at each pixel.\n", " - 7.5 HDR: an HDR image with maximum brightness of 7.5 W.\n", " - 120 HDR: an HDR image with maximum brightness of 120 W."
Suggestion:
brightness of 120 W at each pixel
tutorials/configuring_lighting.ipynb line 2423 at r4 (raw file):
"metadata": {}, "source": [ "Although the three images use environment maps with the exact same resolution\n",
FYI The 1-watt image has a light grey background, and the 7.5-w and 120-w images have a black background. Is this expected?
Edit: I think this is addressed in the exposure section below, but probably worth acknowledging this difference here, and defer the explanation to the following section.
tutorials/configuring_lighting.ipynb line 2455 at r4 (raw file):
"energy as the image rendered with the 120-watt environment map. And yet, the\n", "\"dim\" image is not simply a copy of the bright image with each pixel having\n", "1/120th the value of the original. The dimmer image features smooth contours\n",
BTW Maybe it's the size of the image (unfortunate that I can zoom in), but it's difficult to see the contours that are referring to in the dim image.
Code quote:
The dimmer image features smooth contours
tutorials/configuring_lighting.ipynb line 2575 at r4 (raw file):
"background intensity is functionally black and renders as such.\n", "\n", "Judicious use of exposure is critical in producing images that are both\n",
Is it worth mentioning that tuning exposure might still be insufficient to capture the desired quality (e.g., this issue).
tutorials/configuring_lighting.ipynb line 2590 at r4 (raw file):
"\n", "Open source environment maps can be downloaded from repositories like\n", "[Polyhaven](https://polyhaven.com/).\n",
BTW As far as I could tell, Polyhaven .hdr environment maps don't seem to come in the various energy flavors (i.e., 1-watt, 7.5-watt, etc.). Is there a reason for that?
There was a problem hiding this comment.
First pass done.
Reviewable status: 46 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Big question for you before I get to the small details:
Were there any big picture comments?
Missing topics?
Questions that you still don't have answered? Questions you didn't have before the tutorial that you have now? ETc.
Reviewable status: 46 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes
a discussion (no related file):
Previously, rcory (Rick Cory) wrote…
FYI when I start the tutorial, all cells in the notebook show up as already evaluated (and all renderings already shown without running any code). I'm assuming this isn't intentional (since you want the reader to start fresh).
I can easily clear things by going to the top menu and selecting
Cell>All Output>Clearand saving the file.
Actually, that's exactly what I wanted.
SeanCurtis-TRI
force-pushed
the
PR_lighting_tutorial
branch
from
bb28cee to
bec3144
Compare
There was a problem hiding this comment.
First half pass - clear out typos and what not.
Reviewable status: 44 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
tutorials/configuring_lighting.ipynb line 879 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
FYI Not sure if this TODO was left intentionally to be visible in the tutorial? Same for others below.
I'd considered an illustration of normal vs light direction. I now think it may be a bit too pedantic.
Let me know if you think it would be helpful.
tutorials/configuring_lighting.ipynb line 701 at r3 (raw file):
" directed cone limits the directions that rays are emitted. The cone has its\n", " apex at the light `position` and the cone is rotated so its central axis\n", " is aligned with the light's `direction` vector (such the cone's radius\n",
Done.
tutorials/configuring_lighting.ipynb line 706 at r3 (raw file):
" the cone's span, or the angle between the cone's central axis and the\n", " boundary of the cone.\n", " - __Directional light__: This can be thought of a point light that is infinitely\n",
Done.
tutorials/configuring_lighting.ipynb line 717 at r3 (raw file):
"\n", "Light interacts with matter in various ways. While the physics is fascinating\n", "and intricate, and rendering technology typically focuses on the _effects_ and\n",
Done.
tutorials/configuring_lighting.ipynb line 584 at r4 (raw file):
"\n", "For subsequent renderings, we may not do all of these (for example, several\n", "renderings in a can use the same geometry)."
Done.
tutorials/configuring_lighting.ipynb line 637 at r4 (raw file):
"**ATTENTION**: Expect the rendered image to deviate from what is shown in\n", "the meshcat browser. Meshcat introduces an independent light set from those\n", "defined in the `RenderEngine`. Meshcat also uses a PBR illumination model\n",
Done.
tutorials/configuring_lighting.ipynb line 848 at r4 (raw file):
"the full ground box. We see a circular pool of light. The light is circular\n", "because the spot light's cone of light intersects the ground perpendicularly.\n", "If the spot light were tilted, it the pool of light would be elliptical.\n",
Done.
tutorials/configuring_lighting.ipynb line 853 at r4 (raw file):
"A more subtle difference is between the point light and directional light\n", "images. In both images, the ball's appearance is essentially the same. However,\n", "the ground box is different. With the point light, the ground direclty under\n",
Done.
tutorials/configuring_lighting.ipynb line 976 at r4 (raw file):
" \n", "The inverse-squared law is satisified by c = 0, l = 0, q = 1. In practice,\n", "it sometimes becomes convenient to introduce have the intensity decay\n",
Done.
tutorials/configuring_lighting.ipynb line 977 at r4 (raw file):
"The inverse-squared law is satisified by c = 0, l = 0, q = 1. In practice,\n", "it sometimes becomes convenient to introduce have the intensity decay\n", "more slowly by reintroducing constant or linear factors. The three\n",
Done.
tutorials/configuring_lighting.ipynb line 980 at r4 (raw file):
"coefficients should total one.\n", "\n", "**Note**: attenuation does not apply to directional lights as there is\n",
Done.
tutorials/configuring_lighting.ipynb line 1051 at r4 (raw file):
"outputs": [], "source": [ "# Add several arbitrary light; each with an arbitrarily different color do better\n",
Done.
tutorials/configuring_lighting.ipynb line 1051 at r4 (raw file):
"outputs": [], "source": [ "# Add several arbitrary light; each with an arbitrarily different color do better\n",
Done.
tutorials/configuring_lighting.ipynb line 1124 at r4 (raw file):
"surfaces that would otherwise have been lit. In rendering, we don't get\n", "occlusion for free. We create shadows through a variety of approximations. The\n", "spot light's light cone is our first example. In the real world, a spot light's\n",
Done.
tutorials/configuring_lighting.ipynb line 1285 at r4 (raw file):
"\n", "- The shadow cast from the directional light is smaller than that of the spot\n", " light Not only that, it even appears smaller than the ball.\n",
Done.
tutorials/configuring_lighting.ipynb line 1286 at r4 (raw file):
"- The shadow cast from the directional light is smaller than that of the spot\n", " light Not only that, it even appears smaller than the ball.\n", " - The directional shadow *is* definitely smaller than the point light shadow.\n",
Done.
tutorials/configuring_lighting.ipynb line 1638 at r4 (raw file):
"things closer. When the ball is closer to the camera than its shadow, it will\n", "appear larger. When the shadow is closer to the camera than the ball, the\n", "shadow will appear bigger. THe important thing to note is that these shadows are\n",
Done.
tutorials/configuring_lighting.ipynb line 1657 at r4 (raw file):
"light properties doesn't have the effect you expect.\n", "\n", "Again, this discussion is going bias heavily towards intuition and\n",
Done.
tutorials/configuring_lighting.ipynb line 1683 at r4 (raw file):
"direction _l_. The three views _v₀_, _v₁_, and _v₂_ form half-angle\n", "vectors _h₀_, _h₁_, and _h₂_, respectively. Only _h₁_ aligns with\n", "_n_, so only view _v₀_ receives the specular reflection from the\n",
Done.
tutorials/configuring_lighting.ipynb line 1686 at r4 (raw file):
"light source.\n", "\n", "In the real world, surface reflectivity can vary widely some crisp\n",
Done.
tutorials/configuring_lighting.ipynb line 1813 at r4 (raw file):
"surface (its normal direction), _and_ the direction from the surface to\n", "the camera.\n", "4. When like strikes a surface at a \"grazing\" angle, the surface exhbits\n",
Done.
tutorials/configuring_lighting.ipynb line 1863 at r4 (raw file):
"reflections, the effect is typically more noticable on dielectric\n", "materials, because their _base_ reflectivity is generally much lower\n", "than of metals.\n",
Done.
tutorials/configuring_lighting.ipynb line 1951 at r4 (raw file):
"across the two spheres. The metallic sphere's \"hot spot\" is\n", "larger and somewhat oversaturated in the center (we'll deal\n", "with the oversaturatin when we discuss [exposure](#exposure)\n",
Done.
tutorials/configuring_lighting.ipynb line 1992 at r4 (raw file):
"\n", "\n", "**Figure:** the physical environment.\n",
Done.
tutorials/configuring_lighting.ipynb line 2007 at r4 (raw file):
"\n", "\n", "**Figure:** the environment map drawn from the physical scene shown above."
Done.
tutorials/configuring_lighting.ipynb line 2224 at r4 (raw file):
"such specular reflection.\n", "\n", "But this image illustrates to additional phenomena: Fresnel\n",
Done.
tutorials/configuring_lighting.ipynb line 2235 at r4 (raw file):
"so we observe a more intense reflection.\n", "\n", "Both spheres have a reddish tint on that covers a large portion\n",
Done.
tutorials/configuring_lighting.ipynb line 2244 at r4 (raw file):
"some portion gets refracted _into_ the material, bounces around\n", "and, finally, some remaining portion is emitted again. The\n", "_total_ diffuse affect is the sum of all such light. If\n",
Done.
tutorials/configuring_lighting.ipynb line 2248 at r4 (raw file):
"it must be included in the diffuse calculation.\n", "\n", "In this case, for any givn point on the sphere, fully half of\n",
Done.
tutorials/configuring_lighting.ipynb line 2314 at r4 (raw file):
" intensity light sources manifesting as dark colors to film and sensors. As\n", " counter-intuitive as it might seem, a pixel with normalized color\n", " (0.01, 0.01, 0.01) image is putting out 1 W of light, just like a white\n",
Done.
tutorials/configuring_lighting.ipynb line 2335 at r4 (raw file):
"\n", " - PNG: a normalized image with brightness of 1 W at each pixel.\n", " - 7.5 HDR: an HDR image with maximum brightness of 7.5 W.\n",
Done
I went the other way; I felt that the initial "at each pixel" was probably unhelpful. If an image has a maximum brightness, then it has that.
tutorials/configuring_lighting.ipynb line 2336 at r4 (raw file):
" - PNG: a normalized image with brightness of 1 W at each pixel.\n", " - 7.5 HDR: an HDR image with maximum brightness of 7.5 W.\n", " - 120 HDR: an HDR image with maximum brightness of 120 W."
Done.
There was a problem hiding this comment.
Overall the tutorial is very thorough and helpful. Thank you for putting this together. A couple of big picture comments:
- Each individual section of the tutorial was filled with a lot of information, and I think you did a nice job of exploring the various knobs available. Toward the end though, I found myself wondering if it might be helpful to conclude with a "Putting It All Together" section, that takes the reader through a short (but real) example of lighting an actual scene (e.g., the sports car scene at the beginning of the tutorial), showing the sequence of steps you'd go through for producing nice aesthetics. FTR, I think the tutorial is great as is, but tying it up with a more tangible example would be the cherry on top.
- This is not related specifically to this PR, but I really wish we had an interactive camera configuration tool/GUI, that would allow you to visualize different rendering and camera configurations on a target scenario quickly (e.g., like sliders that instantaneously take effect in a renderer window). There are a lot of parameters to play with, and a typical scene with complex robots/environments still requires a fair amount of fiddling with all these parameters, even if you're already familiar with all the knobs. Changing numbers in a yaml and then running a sim just becomes tedious. Again, not directly related to this PR, but worth mentioning.
Reviewable status: 44 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Actually, that's exactly what I wanted.
Well, okay then!
Previously, rcory (Rick Cory) wrote…
I may be misinformed in that regard. Do you feel it would be better if it's base state didn't include the outputs? I have no strong opinion. As a user of the tutorial, which would be better for you? |
There was a problem hiding this comment.
Reviewable status: 44 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I may be misinformed in that regard. Do you feel it would be better if it's base state didn't include the outputs? I have no strong opinion. As a user of the tutorial, which would be better for you?
I just realized that when browsing through the Drake repo on GitHub, having the outputs already present is beneficial (if I want to just browse the tutorial without running any code). You could say the same thing about running the binary...maybe I just want to browse it. So, I think I'm with you now.
I so think if you declare to the reader up front that the intention is to have a base state with outputs already evaluated, that should be sufficient. At first, I thought I had done something that caused all the cells to evaluate, then I realized that was how the notebook was committed.
SeanCurtis-TRI
force-pushed
the
PR_lighting_tutorial
branch
from
eb87f25 to
733421d
Compare
There was a problem hiding this comment.
Putting it all together
I'm glad you said that. I was heavily considering doing that. And then, at the last minute, having been through by umpteenth draft, I copped out and threw in the car image. The real intent was to populate a scene, pick an .hdr image and then iteritively tweak the lighting. So, that seems to coincide with what you're suggesting.
interactive camera configuration tool/GUI
I know what you mean. I started dabbling, but there hasn't been a high priority. :( In the meantime, if I had to do something akin to that, I'd basically start with this tutorial. Populate the scene and then do what this code does -- add/remove render engines while tweaking the parameters. They render fast enough that it could work to interactively tweak settings. (I had a pure python version that gave me instantaneous connection to Meshcat, such that the rendering would keep updating every time I moved the camera.
Reviewable status: 36 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/configuring_lighting.ipynb line 701 at r3 (raw file):
Previously, rcory (Rick Cory) wrote…
I know what you mean, but this wording threw me off at first. The illustration shows multiple vectors originating from the spot light's center, so when you say "its central axis is aligned with the light's direction", it might be clearer to to say "its central axis is aligned with the light's central direction vector".
I'm going to go out on a limb and assume that the markup on words like position, direction, cone_angle don't stand out strongly enough to make them as proper parameters. I'll see what I can do to clarify.
I took a pass and was a bit more explicit about named parameters. Let me know if that helped.
tutorials/configuring_lighting.ipynb line 48 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
BTW I believe there might also be performance trade-offs (i.e., PBR would be slower)?
While that may be strictly true, I don't think a human would notice the difference. Although, this did make me add some guidance about shadow-castling lights. That can produce human discernible differences in rendering performance.
See below.
tutorials/configuring_lighting.ipynb line 52 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
nit while I'm sure these principles are important, I'm not sure they add much value to this tutorial? Should I care about these details, or would pointing to a Wikipedia article on PBR (for those interesting in learning more) be sufficient?
Edit: I see that you've linked to the Filament PBR documentation below - which is probably sufficient.
Simplified.
tutorials/configuring_lighting.ipynb line 93 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
BTW to clarify, do you mean so long as a single glTF file is referenced in a model, that is sufficient to trigger PBR?
Also, what if someone uses a single glTF as a (
Convex) collision geometry, but not otherwise referenced in any illustration roles? I presume PBR would only trigger if illustration (or perception roles?) use glTF?
Done
Clarified
tutorials/configuring_lighting.ipynb line 1117 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
BTW in the discussion of shadows below, the topic of shadow edge sharpness was never discussed. I don't believe this is yet supported (or exposed?) in Drake, but probably worth mentioning at some point.
I think I know what you mean. I've added some heads up on that.
tutorials/configuring_lighting.ipynb line 1212 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
But why not?? I found myself wondering why the shadow itself for point and spot lights (in this example) wouldn't end up being the same?
Edit: I see that this finally gets answered in the "Spot light limitations" section. Might be worth referring the reader to that section here.
Done.
tutorials/configuring_lighting.ipynb line 2455 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
BTW Maybe it's the size of the image (unfortunate that I can zoom in), but it's difficult to see the contours that are referring to in the dim image.
I think, perhaps, the word "contour" may have led you in the wrong direction. I took a different stab at making the point.
tutorials/configuring_lighting.ipynb line 2575 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
Is it worth mentioning that tuning exposure might still be insufficient to capture the desired quality (e.g., this issue).
Done.
tutorials/configuring_lighting.ipynb line 2590 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
BTW As far as I could tell, Polyhaven
.hdrenvironment maps don't seem to come in the various energy flavors (i.e., 1-watt, 7.5-watt, etc.). Is there a reason for that?
Because those are images of real environments.
I think the real problem here is that my tutorial has led you to expect such images. So, I need to figure out which language led you down the garden path and put up some guard rails.
tutorials/configuring_lighting.ipynb line 1987 at r4 (raw file):
"We'll start with a simple environment map and a mirror ball.\n", "\n", "The image below shows a ball in a simple environment. The mirror ball sites on\n",
Done.
SeanCurtis-TRI
force-pushed
the
PR_lighting_tutorial
branch
from
733421d to
909a908
Compare
There was a problem hiding this comment.
Reviewable status: 36 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, rcory (Rick Cory) wrote…
I just realized that when browsing through the Drake repo on GitHub, having the outputs already present is beneficial (if I want to just browse the tutorial without running any code). You could say the same thing about running the binary...maybe I just want to browse it. So, I think I'm with you now.
I so think if you declare to the reader up front that the intention is to have a base state with outputs already evaluated, that should be sufficient. At first, I thought I had done something that caused all the cells to evaluate, then I realized that was how the notebook was committed.
In drake git master, we always clear the outputs (and are supposed to have a linter that yells if they are not cleared). When deploying to Deepnote, we fill them in again so its' easy to browse as a static site.
Having the outputs here during PR review is okay, but we must zap them before landing on master.
There was a problem hiding this comment.
Reviewable status: 37 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
In drake git master, we always clear the outputs (and are supposed to have a linter that yells if they are not cleared). When deploying to Deepnote, we fill them in again so its' easy to browse as a static site.
Having the outputs here during PR review is okay, but we must zap them before landing on master.
Sounds good. I've marked myself as blocking on this.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/configuring_lighting.ipynb line 701 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm going to go out on a limb and assume that the markup on words like
position,direction,cone_angledon't stand out strongly enough to make them as proper parameters. I'll see what I can do to clarify.I took a pass and was a bit more explicit about named parameters. Let me know if that helped.
Yup, I had missed those markup associations. Your new wording is much better. Thanks.
tutorials/configuring_lighting.ipynb line 879 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'd considered an illustration of normal vs light direction. I now think it may be a bit too pedantic.
Let me know if you think it would be helpful.
The explanation was clear to me. I don't think it's necessary.
tutorials/configuring_lighting.ipynb line 1117 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I think I know what you mean. I've added some heads up on that.
Yup, you got it. That's what I was referring to.
tutorials/configuring_lighting.ipynb line 1951 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Done.
The change seems to not have made it through.
tutorials/configuring_lighting.ipynb line 2590 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Because those are images of real environments.
I think the real problem here is that my tutorial has led you to expect such images. So, I need to figure out which language led you down the garden path and put up some guard rails.
I think I now realized my misunderstanding. Your 7.5 HDR and 120 HDR environment map examples were engineered for those max energy levels. Somehow my brain interpreted that HDR images in the wild would come in different flavors, but of course the max energy level in an HDR image will strictly depend on the lighting present in the environment that the HDR image is capturing. Only the normalized image will guarantee a fixed max energy level of 1W. Is that correct?
tutorials/configuring_lighting.ipynb line 853 at r6 (raw file):
"The most obvious difference is the spot light's limit. It doesn't illuminate\n", "the full ground box. We see a circular pool of light. The light is circular\n", "because the spot light's cone of light intersects the ground perpendicularly.\n",
nit To be consistent
Suggestion:
light's light cone
tutorials/configuring_lighting.ipynb line 2462 at r6 (raw file):
"The final images clearly depend significantly on the light levels in the\n", "environment maps. Does this mean that there are some environment maps that are\n", "simply unusable? They have too much energy? Good news! We don't. We are free to\n",
Suggestion:
No, it doesn't.
tutorials/configuring_lighting.ipynb line 2484 at r6 (raw file):
"balls are _not_ silhouettes. There is still visible gradation of light\n", "communicating the rounded shape of the balls. In fact, the dim, 1-watt image\n", "shows more more of the spherical shape for the bottom row of balls than the\n",
Suggestion:
more
There was a problem hiding this comment.
Reviewed 2 of 20 files at r3, 5 of 5 files at r4.
Reviewable status: 12 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Sounds good. I've marked myself as blocking on this.
Yeah, reviewing is difficult with the outputs already there, too. I would clear it and push sooner than later, please.
tutorials/BUILD.bazel line 7 at r6 (raw file):
filegroup( name = "resources",
Tutorials cannot use FindResourceOrThrow. I believe the best answer is to just put these into drake_models with a good subdir name e.g. package://drake_models/tutorials/sphere.gltf or package://drake_models/tutorials/configuring_rendering_lighting/sphere.gltf.
tutorials/index.ipynb line 29 at r6 (raw file):
"- [PyPlot Animation with MultibodyPlant](./pyplot_animation_multibody_plant.ipynb)\n", "- [Rendering MultibodyPlant](./rendering_multibody_plant.ipynb)\n", "- [Configuring Rendering Lighting](./configuring_lighting.ipynb)\n",
BTW Probably we should have the title here, the filename, and the notebook's first header all have the same wording? (Should we rename this file?)
tutorials/images/env_color_room.blend line 0 at r6 (raw file):
Working
I'm still working on how to deploy these images into our channels, I'll post back on that soon.
tutorials/images/env_color_room.blend line 0 at r6 (raw file):
Assuming (per the thread immediately above) that these files can stay here in git (with some changes to BUILD files), then here's some relevant discussions / defects. Possibly not worth trying to change these until we know how BUILD will work, though.
I suppose the extra files here (not cited by the tutorial, like this one) are to preserve the source material so that we can regenerate the pngs again later if need be? That's fine in principle, but:
(1) We should logically separate them somehow to make that more clear, e.g. images/ vs images/sources/ or something.
(2) The blend files are really big. Is there any way to make them smaller to not choke up our git clones?
tutorials/configuring_lighting.ipynb line 39 at r6 (raw file):
"\n", "<a id=\"what_is_pbr\"></a>\n", "## Physically-based Rendering (PBR)\n",
BTW My keyboard navigation / scrolling of the notebook would have been nicer if this section were it's own cell, separate from the overview & TOC.
tutorials/configuring_lighting.ipynb line 571 at r6 (raw file):
"source": [ "if meshcat.GetTrackedCameraPose() is None:\n", " raise RuntimeError(\n",
This raise fails in CI, and will probably be annoying for the release engineer every month. Maybe just logging.error instead of raising?
There was a problem hiding this comment.
Reviewable status: 12 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
tutorials/images/env_color_room.blend line at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
I'm still working on how to deploy these images into our channels, I'll post back on that soon.
Add a file tutorials/images/publish_images_via_doxygen.h with the following content, and change the ! image URLs to point to the drake.mit.edu doxygen website, like we did for hydro tutorial.
If you want local reviewers to be able to see the images, you might need to open an ahead-of-time PR that just adds the images (and the header).
/** @file
(Ignore this file.)
These images are added to Doxygen in support of
<a href="https://github.com/RobotLocomotion/drake/blob/master/tutorials/README.md">Drake's Tutorials</a>.
Refer that that link for details.
@image html "tutorials/images/env_color_room_in_situ.png"
@image html "tutorials/images/light_types.png"
@image html "tutorials/images/pbr_car.jpg"
@image html "tutorials/images/shadow_map_resolution.png"
@image html "tutorials/images/simple_reflection.png"
*/
This includes a number of resource files (illustrations, models, environment maps, etc.) In addition, it includes the source files from which these where generated (where reasonable).
SeanCurtis-TRI
force-pushed
the
PR_lighting_tutorial
branch
from
909a908 to
690702f
Compare
There was a problem hiding this comment.
Putting it all together
I remember why I bailed last time. Drake has a paucity of resources. We've got robot arms and manipulands, but otherwise, we have very little in terms of "setting the stage". I'll push it a bit further, to see if I can come up with something....
Reviewable status: 7 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yeah, reviewing is difficult with the outputs already there, too. I would clear it and push sooner than later, please.
Curiously, I had exactly the opposite view. For my money, the images in the PR are the ones I expect a user to see the first time through. When you execute the code and the same image blinks away and then comes back, it shows you that code matches my expectation.
But I'll go ahead and remove images in the next revision.
tutorials/BUILD.bazel line 7 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Tutorials cannot use
FindResourceOrThrow. I believe the best answer is to just put these intodrake_modelswith a good subdir name e.g.package://drake_models/tutorials/sphere.gltforpackage://drake_models/tutorials/configuring_rendering_lighting/sphere.gltf.
Done
tutorials/index.ipynb line 29 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Probably we should have the title here, the filename, and the notebook's first header all have the same wording? (Should we rename this file?)
Done
tutorials/images/env_color_room.blend line at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Add a file
tutorials/images/publish_images_via_doxygen.hwith the following content, and change the!image URLs to point to the drake.mit.edu doxygen website, like we did for hydro tutorial.If you want local reviewers to be able to see the images, you might need to open an ahead-of-time PR that just adds the images (and the header).
/** @file (Ignore this file.) These images are added to Doxygen in support of <a href="https://github.com/RobotLocomotion/drake/blob/master/tutorials/README.md">Drake's Tutorials</a>. Refer that that link for details. @image html "tutorials/images/env_color_room_in_situ.png" @image html "tutorials/images/light_types.png" @image html "tutorials/images/pbr_car.jpg" @image html "tutorials/images/shadow_map_resolution.png" @image html "tutorials/images/simple_reflection.png" */
I'll do this. For now, I propose to leave the images in tree (for the review process). Iterating on content will be much easier if the changes are immediately available.
Once we are all happy with the content of the images, I'll create the proposed PR, update this, and then we can defer merging until the documentation has gone through.
tutorials/images/env_color_room.blend line at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Assuming (per the thread immediately above) that these files can stay here in git (with some changes to BUILD files), then here's some relevant discussions / defects. Possibly not worth trying to change these until we know how BUILD will work, though.
I suppose the extra files here (not cited by the tutorial, like this one) are to preserve the source material so that we can regenerate the pngs again later if need be? That's fine in principle, but:
(1) We should logically separate them somehow to make that more clear, e.g.
images/vsimages/sources/or something.(2) The blend files are really big. Is there any way to make them smaller to not choke up our git clones?
The blend files have moved; only the .svg files remain. I have now made the sources subdirectory.
tutorials/configuring_lighting.ipynb line 1951 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
The change seems to not have made it through.
Done done. This time for real.
tutorials/configuring_lighting.ipynb line 2590 at r4 (raw file):
Previously, rcory (Rick Cory) wrote…
I think I now realized my misunderstanding. Your 7.5 HDR and 120 HDR environment map examples were engineered for those max energy levels. Somehow my brain interpreted that HDR images in the wild would come in different flavors, but of course the max energy level in an HDR image will strictly depend on the lighting present in the environment that the HDR image is capturing. Only the normalized image will guarantee a fixed max energy level of 1W. Is that correct?
You got it. I tried massaging the language where the hdr images are introduced to forestall the previous expectation.
I've clarified a bit more about the origin of the environment maps.
tutorials/configuring_lighting.ipynb line 853 at r6 (raw file):
Previously, rcory (Rick Cory) wrote…
nit To be consistent
Well, we have two instances of "light's light cone" and two instances of "cone of light"
As I slightly dislike the "light's light" pattern slight more than the other, I've made things consistent in the other direction.
tutorials/configuring_lighting.ipynb line 2462 at r6 (raw file):
"The final images clearly depend significantly on the light levels in the\n", "environment maps. Does this mean that there are some environment maps that are\n", "simply unusable? They have too much energy? Good news! We don't. We are free to\n",
I just cut it.
tutorials/configuring_lighting.ipynb line 2484 at r6 (raw file):
"balls are _not_ silhouettes. There is still visible gradation of light\n", "communicating the rounded shape of the balls. In fact, the dim, 1-watt image\n", "shows more more of the spherical shape for the bottom row of balls than the\n",
So, what you're saying is that you want less more?
There was a problem hiding this comment.
A long awaited update has finally landed. PTAL.
Reviewable status: 7 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory, missing label for release notes (waiting on @SeanCurtis-TRI)
jwnimmer-tri
added
the
release notes: feature
There was a problem hiding this comment.
Reviewed 12 of 20 files at r7, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),rcory
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Curiously, I had exactly the opposite view. For my money, the images in the PR are the ones I expect a user to see the first time through. When you execute the code and the same image blinks away and then comes back, it shows you that code matches my expectation.
But I'll go ahead and remove images in the next revision.
(The main problem is the clutter in Reviewable, making it more difficult to navigate inside the page.)
tutorials/images/env_color_room.blend line at r6 (raw file):
For now, I propose to leave the images in tree (for the review process). Iterating on content will be much easier if the changes are immediately available.
Fine by me.
tools/workspace/drake_models/repository.bzl line 9 at r7 (raw file):
name = name, repository = "SeanCurtis-TRI/models", commit = "65f201d0c05e33e2d2ed0a225143aced5b7c63e2",
Working
Reminder for me to check the final sha here later.
This includes a number of resource files (illustrations, models, environment
maps, etc.) In addition, it includes the source files from which these
where generated (where reasonable).
This change is