Lighting support in the batch renderer #1970

mosra · 2022-12-20T00:47:49Z

Motivation and Context

Things left to do:

How to enable this within the Python viewer, for @0mdc:

Pass an appropriate maxLightCount to the renderer configuration, as the default is 0, which flat-shades everything.
There's no step 2. Just ensure you're using correct composites and that an appropriate light setup is passed through.

How Has This Been Tested

Look at those pretty pictures! 🔴 🟡

0mdc · 2022-12-20T01:41:17Z

Looks good so far! Nice progress.

mosra · 2022-12-20T19:30:13Z

This PR is now ready for a review. What's left is ~~implementing the distinction between point vs directional lights, and~~ updating the compositor scripts in #1969 to include normals and mark only the acually baked materials as flat.

A horrible screenshot for ~~your viewing pleasure~~ -- the overlapping objects are an issue with the input data, and the extremely overblown lighting as well. I'll dive into this and try to create some more visually pleasing setup.

src/esp/sim/BatchReplayRenderer.cpp

mosra · 2022-12-20T22:06:41Z

#1969 is now updated with lighting-aware composite creation and this PR is thus considered finished from my side. Output with the new composites for reference (above was using the loose glTF files) -- the main difference is that the Replica room model is now correctly using a flat-shaded material and thus isn't affected by lighting, which was making it look weird.

Non-ugly screenshots posted on Slack.

0mdc · 2022-12-21T14:36:56Z

src/esp/sim/BatchReplayRenderer.cpp

+        }
+
+        renderer_.lightColors(sceneId_)[lightId] = light.color;
+        // TODO range, once Habitat has that


0mdc

Looks good to me!

eundersander

Good progress. My comments are mostly just aimed at gaining understanding.

eundersander · 2023-01-06T16:04:20Z

src/esp/gfx_batch/Renderer.h

+   * By default no lights are used, i.e. flat-shaded rendering.
+   * @see @ref Renderer::maxLightCount()
+   */
+  RendererConfiguration& setMaxLightCount(Magnum::UnsignedInt count);


How about a comment with some guidance for how to pick this number? E.g. Assuming a single draw can possibly be the entire scene, then the max count should probably be similar to the expected max light count across all scenes (is that even true?).

It's the max number of lights applied per draw.

The semantic will change slightly once light culling is in (for which it's essential that the lights contain range info, which they do in glTF files but not yet in Habitat-supplied lights). With light culling, this will be an upper bound of lights used per draw (picking the brightest from the set, ignoring the rest) and the number of lights in total will be handled internally, without any need for user-specified settings.

Currently without the culling it's both the max number of lights applied per draw and the max number in total, because that's the same thing. I'll update the docs when light culling is a thing.

eundersander · 2023-01-06T16:09:12Z

src/esp/gfx_batch/Renderer.cpp

+        const auto& phongMaterial =
+            material->as<Mn::Trade::PhongMaterialData>();
+        materials[i] = Mn::Shaders::PhongMaterialUniform{}
+                           .setAmbientColor(phongMaterial.diffuseColor() * 0.1f)


Ug, what is this 0.1? Are we hard-coding the amount of ambient light?

Yes -- see this Slack post for details.

I wanted to achieve an acceptable look until there's a proper PBR pipeline where one doesn't need to hand-tune this kind of thing anymore. Same for specular highlights. The materials are all encoded as PBR, which means there's no info about "classic" specular highlights or ambient anymore, and so those are completely under "artist control", let's say.

How about re-using LightSetup.h getAmbientLightColor? If we matched that value, would it be easier to get visual parity with the classic renderer?

I could look into that, yup -- I remember you spent a considerable amount of time on the PBR -> Phong conversion, so it would be a shame to ignore it.

How should I deal with the original requirement that gfx_batch stays completely isolated from the rest of Habitat? (I was considering putting some PBR -> Phong conversion directly into Magnum, but that time would probably be better spent on actually making PBR work.)

But, re visual parity -- the recent Floorplanner discussion made me think I should maybe try something from scratch instead of trying to match how it looks in the classic renderer 😅

the original requirement that gfx_batch stays completely isolated from the rest of Habitat?

Yep. So this would be some caller like replayer.cpp passing LightSetup.h getAmbientLightColor to the batch renderer, maybe via a new public method or the Configuration struct.

the recent Floorplanner discussion made me think I should maybe try something from scratch instead of trying to match how it looks in the classic renderer
I don't have the full context here, but I think it'll be valuable going forward to be able to swap between the classic and batch renderer, with Mikael working to reach parity.

Oh, wait, this is about the light ambient color, not the per-material ambient color, I missed that.

So, just to clarify we're on the same page -- the aspect of diffuse / base color used partially for ambient is fine in this case, and you're only concerned about the factor being hardcoded, right? Not about the fact that the same texture is used for both.

Having it configurable from the light makes sense, but it'll be a global factor (i.e., there's no way to have different lights with different ambient factors affecting different meshes, the ambient factors will get all summed up and used as a single value). But that should be fine, I think?

you're only concerned about the factor being hardcoded, right?

Yes.

Not about the fact that the same texture is used for both.

No problem here. I expect this.

Somewhat related, I know we've disagreed in the past a bit on this nuance of the Phong lighting model. My preference is for a material's ambient color to generally match its diffuse color, and for that material ambient color to get multiplied with an ambient light color at runtime (every frame), such that you could hypothetically vary the color/amount of ambient light in the scene every frame. Whereas I know you've spoke in the past of preferring to pre-multiply these two and basically bake them into the material's ambient color. I'm too lazy to track down that Slack thread from a year ago, but more specifically, you said that many 3D assets in the wild already do this pre-baking by convention. Anyway, we don't have to debate this now or change the current code in this respect.

Having it configurable from the light makes sense, but it'll be a global factor

Exactly, a global factor, not per light. I think of ambient light color as sorta like the cosmic background radiation. It's a constant amount of light shining in all directions, everywhere in the scene. A lazy proxy for e.g. env-map-based light.

nuance of the Phong lighting model

Yes, I remember -- and together with the global ambient factor I think I'm now taking your approach here :) My original reasoning (and it was possibly even more than a year ago) was that there are existing "classically lighted" models that set the ambient color on their own (or even have dedicated ambient textures), and so ignoring that and overriding it with some fraction of diffuse would lead to information loss. But now, and especially with the workflow here being that the composite files are changed to use PBR materials only, most models is made with the PBR workflow in the first place, so there isn't any "ambient color" info present anymore and so overriding ambient this way won't lead to any information loss.

In b447462 I added a function to change the ambient factor to not have it hardcoded to 0.1. But due to the way materials are set up, it's currently only possible to change it upfront, not later at runtime from the gfx-replay file. OTOH, the getAmbientLightColor() had it hardcoded too, so not really a runtime value either -- I added a TODO.

eundersander · 2023-01-06T16:10:25Z

src/esp/gfx_batch/Renderer.cpp

+        materials[i] = Mn::Shaders::PhongMaterialUniform{}
+                           .setAmbientColor(phongMaterial.diffuseColor() * 0.1f)
+                           .setDiffuseColor(phongMaterial.diffuseColor())
+            /* Specular not used (and shader compiled with NoSpecular). Much


Can you explain this better? Is the problem that our existing assets have too much spec on their materials? Or is the problem that our point lighting doesn't produce nice-looking highlights?

Same as in case of the ambient, the PBR materials in glTF don't encode any info about "classic" specular so the renderer has to guesstimate and it usually guesses wrong.

From my experience the specular "white spots" were what usually people complained about when seeing the renderer output (such as the recent Floorplanner discussion on Slack), and removing them makes the lighting look significantly less "everything is made out of shiny plastic".

eundersander · 2023-01-06T16:18:29Z

src/esp/gfx_batch/Renderer.cpp

      shaderFlags |= Mn::Shaders::PhongGL::Flag::AmbientTexture |
                     Mn::Shaders::PhongGL::Flag::TextureArrays |
                     Mn::Shaders::PhongGL::Flag::TextureTransformation;
+      /* Enable also a diffuse texture if we're rendering with lights */
+      if (state_->maxLightCount)
+        shaderFlags |= Mn::Shaders::PhongGL::Flag::DiffuseTexture;


To sanity-check: a PBR material's baseColor texture would get loaded once and be set as both the ambient texture and diffuse texture for the corresponding Phong material, yes?

eundersander · 2023-01-06T16:22:17Z

src/esp/gfx_batch/Renderer.cpp

+          .setNormalMatrix(state_->absoluteTransformationsSorted[i]
+                               .transformationMatrix.normalMatrix())
+          // TODO light culling should happen here
+          .setLightOffsetCount(0, scene.lights.size());


Where is maxLightCount enforced?

On line 831, in setLightCount() passed to Shaders::PhongGL::Configuration -- it's a shader-compile-time setting, not a uniform input.

As I said above, this will change slightly when light culling is done -- the compile-time light count will be handled internally, and the max per-draw light count will be limiting the count of lights used per draw, picking just the ones with most influence.

eundersander · 2023-01-06T16:29:55Z

src/esp/gfx_batch/Renderer.h

+   * @brief Add a light
+   * @param sceneId         Scene ID, expected to be less than
+   *    @ref sceneCount()
+   * @param nodeId          Node ID to inherit transformation from, returned


Do we support the use case of a gltf mesh hierarchy with a light attached to some subnode? E.g. a lamp asset with a light attached, where the subnode offsets from the base of the lamp to the location of the bulb.

Er I'm guessing we do and it doesn't require an explicit addLight. I'm guessing adding that mesh hierarchy also adds the child light. Which is a whole can of worms we haven't really discussed (adding lights embedded in a gltf: the legacy renderer doesn't support this, right?).

Do we support the use case of a gltf mesh hierarchy with a light attached to some subnode?

Yes, although not everything is in place yet, that's what I designed for, to make it easier to author scenes with uniform lighting, as it's significantly easier to tune light intensities/ranges in Blender and export a self-contained glTF than supplying them externally via a JSON config, or having to explicitly add also a light every time you add a lamp to the scene.

I want to treat those as being a part of the asset (and thus not something to be explicitly supplied by the simulator apart from a high-level use like "hold a torch" or "add a lamp there"), same as e.g. emissive textures are. And eventually the datasets will contain also lightprobe locations and other light-related data needed by PBR, which would otherwise have to be proxied through Habitat's data pipeline.

adding lights embedded in a gltf: the legacy renderer doesn't support this, right?

I was pushing for loading lights from the glTF several times (because, again, it's significantly easier to author the assets that way), all the scaffolding is in place, but probably nobody did the work to hook it up within habitat. ¯\_(ツ)_/¯

I was pushing for loading lights from the glTF several times

Ok. But please don't go rogue on us! Let's get consensus on this stuff. For the classic renderer, lights come from the JSON metadata. The new batch renderer should work the same way, at least for now. We don't want to introduce confusion where someone is switching between renderers and wondering why extra lights start showing up or disappearing.

lights come from the JSON metadata. The new batch renderer should work the same way

It does, it consumes lights from the JSON metadata, via the gfx-replay :) But I see what you mean -- when I get to hooking this up, I'll add a flag to skip lights coming from the input file, similarly to how there's a flag to skip texture.

eundersander · 2023-01-06T16:32:15Z

src/esp/gfx_batch/Renderer.h

+   *
+   * Returns a view on colors of all lights in given scene. Desired usage is to
+   * update only colors at indices returned by @ref addLight(), updating colors
+   * at other indices is possible but could have unintended consequences.


Just curious, how do we end up with lights in here other than via addLight?

From lights being implicitly added via addHierarchy(), as discussed above.

To be clear: is this fully hooked up? If we add a hierarchy that includes a light, will the light get added?

Not yet -- I didn't find time to actually equip datasets with lights to have something to test with and didn't want to add completely uncovered code to this PR, so it's not here here. The "compositor" in #1969 and the glTF exporter also don't handle those for a similar reason, but both are relatively trivial pieces of code.

Once I have data to test with, I will be definitely bugging everyone on Slack with this feature ;)

eundersander · 2023-01-06T16:34:51Z

src/esp/sim/BatchReplayRenderer.cpp

+
+        const std::size_t nodeId = renderer_.addEmptyNode(sceneId_);
+
+        /* Clang Tidy, you're stupid, why do you say that "lightId" is not


an accidentally zero-initialized variable is also a bug, you know?

wut

@mosra Just add NOLINTNEXTLINE comment then. :)

Because if the variable was meant to be initialized to something, but by accident wasn't, the zero-initialization would hide that bug, causing the program to behave seemingly fine but incorrect. While not having it initialized at all (kept as a random memory) could possibly lead to a random OOB assertion down the line, or cause the program to behave randomly, making it easier to discover the bug.

At least in my experience it was always like this, bugs caused by random memory were easier to discover than bugs where the variables were zero-initialized by accident.

cause the program to behave randomly, making it easier to discover the bug

Not my experience. Nondeterministic bugs are scary. E.g. by leaving it uninitialized, it gets initialized to zero in debug builds but something random and nonzero in release builds, so the entire EA Sports Nascar 2005 eng team spends the entire afternoon tracking down why only the release build is broken. :)

Counterpoint to a counterpoint: GCC 12 has -ftrivial-auto-var-init=pattern which initializes local variables in debug builds to some bit pattern. MemorySanitizer can do something similar for the heap as well, and maybe for local variables too.

So if the variable is accidentally left uninitialized, these options can be used to make the bug behave deterministically. But if it gets zero-initialized because Clang Tidy demanded to, and then accidentally not set to the value it should have, it's harder to discover, as it would behave as a "regular" value and won't trigger any assertions or sanitizer reports. In my opinion. But my general opinion is fail fast & fail hard, which is probably something completely undesirable in game development :)

If this were my code, I'd still prefer to initialize to an aggressively-invalid value like -1 or something, but I don't feel too strongly and you've clearly given this some thought. Fine to leave this as-is!

eundersander · 2023-01-06T16:37:35Z

src/esp/sim/BatchReplayRenderer.cpp

+          lightId = renderer_.addLight(sceneId_, nodeId,
+                                       gfx_batch::RendererLightType::Point);
+        } else {
+          /* The matrix will be partially NaNs if the light vector is in the


Well that sounds pretty shady. You're really okay with NaNs floating around here? What if one day we want to assert that all transformations are orthonormal?

In past codebases, I used to have a "getArbitraryOrthogonalVector" and/or "getArbitraryLookatTransform" for this kinda thing.

I'm okay with that, yes. I view it as the NaNs signifying that the X/Y basis is unimportant and only Z matters, and it works as expected even in presence of matrix transformations -- NaNs stay where they should stay:

>>> Matrix4.rotation_x(Deg(30.0))@\ Matrix4.look_at(Vector3(), Vector3.y_axis(), Vector3.y_axis()) Matrix(-nan, -nan, 0, 0, -nan, -nan, -0.866025, 0, -nan, -nan, -0.5, 0, -nan, -nan, 0, 1)

If it eventually causes asserts for whatever reason, I can fix those, but even orthonormal transforms are sometimes impossible such as in this case of meshes flattened along a certain axis. And inventing an always-orthogonal transformation just to avoid NaNs would only mean having extra code to test and it'd be all discarded in the end anyway.

Some bold ideas from Mosra today, haha. Another counterpoint: what if someone tries to debug-render all the node coordinate frames as red/green/blue axes or whatever? Now you have NaNs in vertices you're submitting to the driver.

Hmm, that's right -- a mesh transformed by this matrix would be all NaNs. A visualizer using the base axes directly wouldn't tho, it would show just the blue axis, which is maybe what is meant to be visualized?

I'm taking it too far here, am I :D

it would show just the blue axis, which is maybe what is meant to be visualized?

Ohhhh, that's going to be a mystery rabbit hole for whatever poor junior SWE attempts this visualization.

Speaking of rabbit holes, I don't like dragging out these little code review nits, and I don't want to get into a mode of demanding changes, so I'll ask you to make the final decision here!

I added a TODO -- once I have a convenience utility for this in Magnum, I change to it instead of lookAt().

eundersander

Not sure if you have more changes in progress related to my recent comments, but I feel like our threads are resolved at least philosophically, so I'm giving the green check!

0mdc · 2023-01-09T14:08:56Z

src/esp/sim/BatchReplayRenderer.cpp

+           range asserts in all functions so your "suggestions" are completely
+           unhelpful. */
+        std::size_t lightId;  // NOLINT
+        if (light.vector.z()) {


We want to look at the w component of the light vector to determine its type.

@0mdc I am not following, is this flagging a bug as it is currently written?

Oh, whoops -- yep, that's a bug. Fixing...

Fixed in 3906aaa.

In a preparation for the hierarchy to contain more than just meshes -- such as lights. What is *very amazing* about this change is that it's the exact same number of characters as before, so Clang Format has nothing to wildly rearrange here.

…ayer.

…ght.

Due to how materials are implemented right now, this can't be easily changed at runtime, thus it's not propagated from the replay file.

mosra requested review from aclegg3, eundersander and 0mdc December 20, 2022 00:47

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 20, 2022

mosra force-pushed the let-there-be-light branch 6 times, most recently from f1adc6a to afe4d87 Compare December 20, 2022 19:23

mosra marked this pull request as ready for review December 20, 2022 19:23

Skylion007 reviewed Dec 20, 2022

View reviewed changes

src/esp/sim/BatchReplayRenderer.cpp Outdated Show resolved Hide resolved

mosra force-pushed the let-there-be-light branch 2 times, most recently from 5f6ebb1 to 09f3909 Compare December 20, 2022 20:41

0mdc reviewed Dec 21, 2022

View reviewed changes

0mdc approved these changes Dec 21, 2022

View reviewed changes

Skylion007 approved these changes Dec 21, 2022

View reviewed changes

mosra force-pushed the let-there-be-light branch from dde8bb0 to 5851714 Compare December 21, 2022 16:28

eundersander reviewed Jan 6, 2023

View reviewed changes

eundersander approved these changes Jan 7, 2023

View reviewed changes

0mdc reviewed Jan 9, 2023

View reviewed changes

mosra added 6 commits January 9, 2023 17:54

gfx_batch: support lights.

cfba706

Wire the light support through the gfx-replay APIs, enable it in repl…

78b696b

…ayer.

Clang Tidy You're Stupid

14aa096

Add a TODO for a "less NaNy" lookAt() alternative.

11edc01

Use proper coordinate to distinguish between point and directional li…

3906aaa

…ght.

Improve setMaxLightCount() docs.

384d7ea

mosra force-pushed the let-there-be-light branch from 4ce5bbe to c80bd96 Compare January 9, 2023 17:19

Don't hardcode the ambient factor.

b447462

Due to how materials are implemented right now, this can't be easily changed at runtime, thus it's not propagated from the replay file.

mosra force-pushed the let-there-be-light branch from c80bd96 to b447462 Compare January 9, 2023 17:23

mosra merged commit 4ed8d04 into main Jan 10, 2023

mosra deleted the let-there-be-light branch January 10, 2023 11:10


		const std::size_t nodeId = renderer_.addEmptyNode(sceneId_);

		/* Clang Tidy, you're stupid, why do you say that "lightId" is not

Lighting support in the batch renderer #1970

Lighting support in the batch renderer #1970

Conversation

mosra commented Dec 20, 2022 • edited Loading

Motivation and Context

How Has This Been Tested

0mdc commented Dec 20, 2022 • edited Loading

mosra commented Dec 20, 2022 • edited Loading

mosra commented Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

0mdc left a comment

Choose a reason for hiding this comment

eundersander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eundersander Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

eundersander Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eundersander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra commented Dec 20, 2022 •

edited

Loading

0mdc commented Dec 20, 2022 •

edited

Loading

mosra commented Dec 20, 2022 •

edited

Loading

mosra commented Dec 20, 2022 •

edited

Loading

mosra Jan 6, 2023 •

edited

Loading

mosra Jan 6, 2023 •

edited

Loading

eundersander Jan 7, 2023 •

edited

Loading

eundersander Jan 7, 2023 •

edited

Loading

mosra Jan 6, 2023 •

edited

Loading