Add DebugLineRender utility #1349

eundersander · 2021-06-28T18:34:28Z

Motivation and Context

Singleton utility class for on-the-fly rendering of lines (e.g. every frame). This is intended for debugging or simple UX for prototype apps. The API prioritizes ease-of-use over maximum runtime performance.

replay_playback3.mp4

How Has This Been Tested

integrated into replay_tutorial.py; also ad-hoc testing in viewer.cpp

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

eundersander · 2021-06-28T18:37:28Z

src/esp/gfx/DebugLineRender.cpp

+  // thick, visually-appealing line. Todo: implement thick lines using
+  // triangles, for example https://mattdesl.svbtle.com/drawing-lines-is-hard.
+  // 1.2 is hand-tuned for Nvidia hardware to be just small enough so we don't
+  // see gaps between the individual offset lines.


To be clear: the current thick-line implementation here is sloppy in the sense of not producing perfectly-rendered thick lines. We can upgrade in the future without changing the public API, if we wish.

eundersander · 2021-06-28T18:37:59Z

src/esp/gfx/DebugLineRender.cpp

+  // restore blending state if necessary
+  if (doToggleBlend) {
+    Mn::GL::Renderer::disable(Mn::GL::Renderer::Feature::Blending);
+    // Note: because we are disabling blending, we don't need to restore


@mosra any concern here?

Same as in the other comment, this should be something Magnum eventually takes care of.

For now, this is fine.

eundersander · 2021-06-28T18:39:41Z

src/esp/gfx/DebugLineRender.cpp

+                 "DebugLineRender::flushLines: no GL resources; see "
+                 "also releaseGLResources", );
+
+  bool doToggleBlend = !glIsEnabled(GL_BLEND);


@mosra @bigbike I don't love calling glIsEnabled but I didn't see a Magnum getter.

What I really need here is a way to "push" a render state and later pop it (then I wouldn't need the getter).

I'm open to other suggestions.

Yeah sorry, a nasty pain point.

This should ideally eventually be handled by a scoped renderer state, where you enable what you need and Magnum's state tracker then takes care of cleaning that up again at the end of scope, ideally also not calling into GL more than necessary. Practically we'll probably switch to Vulkan sooner than I get to implement such a feature.

eundersander · 2021-06-28T18:40:52Z

src/esp/gfx/DebugLineRender.cpp

+  // Update shader
+  _glResources->mesh.setCount(_verts.size());
+
+  glDisable(GL_LINE_SMOOTH);  // anti-aliased lines cause artifacts


Feels weird calling OpenGL directly here.

I thought this was some ancient relic and so this isn't even exposed anywhere.

If you want a "less" direct way and something that should be more futureproof, GL::Renderer::disable(GL::Renderer::Feature(GL_LINE_SMOOTH)) and then please add a TODO for me.

I also hit an issue with this not even compiling for Emscripten. I'm starting to think it's pretty unlikely that anyone is going to set this setting to true. I'm just going to remove this line and not worry about it.

Ah, that's why I thought it's an ancient relic, because GLES/WebGL doesn't even define such a thing :D

eundersander · 2021-06-28T18:42:59Z

src/esp/gfx/DebugLineRender.cpp

+}
+
+// return false if segment is entirely clipped
+bool scissorSegmentToOutsideCircle(Mn::Vector3* pt0,


This is used for drawPathWithEndpointCircles as seen in the video.

eundersander · 2021-06-28T18:44:46Z

src/esp/sim/Simulator.cpp

+    // Python may keep around other shared_ptrs to this object, but we need
+    // to release GL resources here.
+    debugLineRender_->releaseGLResources();
+    debugLineRender_ = nullptr;


This is weird. What I really want to do is delete the DebugLineRender instance here. But I can't because I had to share shared_ptrs with Python when doing the bindings. And Python tends to keep around the pointers indefinitely. So we don't actually control when this object gets deleted.

We used a weak_ptr wrapper paradigm to get around this with ManagedObjects. The wrapper does a lock and forwards the command if it succeeds, otherwise warning about the stale reference.

The weak_ptr solution is interesting but feels like overkill for my situation. If a Python user keeps a reference to DebugLineRender after the sim has been closed, and she tries to use it, nothing bad happens.

aclegg3

Overall looks good. Cool feature.

aclegg3 · 2021-06-28T18:38:30Z

src/esp/bindings/GfxBindings.cpp

+  py::class_<DebugLineRender, std::shared_ptr<DebugLineRender>>(
+      m, "DebugLineRender")
+      .def("set_line_width", &DebugLineRender::setLineWidth,
+           R"(See push_transform.)")


Update this doc? I assume it applied to lines following the command?

aclegg3 · 2021-06-28T18:40:15Z

src/esp/bindings/GfxBindings.cpp

+                            const Magnum::Color4&, const Magnum::Color4&>(
+              &DebugLineRender::drawTransformedLine),
+          "from"_a, "to"_a, "from_color"_a, "to_color"_a,
+          R"(Draw a line segment in world-space or local-space (see pushTransform).)")


Suggested change

R"(Draw a line segment in world-space or local-space (see pushTransform).)")

R"(Draw a line segment in world-space or local-space (see pushTransform) with interpolated color.)")

aclegg3 · 2021-06-28T18:43:41Z

src/esp/gfx/DebugLineRender.h

+  /**
+   * @brief Submit lines to the GL renderer. Call this once per frame.
+   * Because this uses transparency, you should ideally call this *after*
+   * submitting opaque scene geomtry.


Suggested change

* submitting opaque scene geomtry.

* submitting opaque scene geometry.

mosra · 2021-06-28T19:49:18Z

src/esp/gfx/DebugLineRender.cpp

+  // Because we render lines multiple times additively in flushLines, we need to
+  // remap alpha (opacity). This is an approximation.
+  constexpr float exponent = 2.f;
+  return Mn::Color4(src.r(), src.g(), src.b(), std::pow(src.a(), exponent));


Code golf:

Suggested change

return Mn::Color4(src.r(), src.g(), src.b(), std::pow(src.a(), exponent));

return {src.rgb(), std::pow(src.a(), exponent)};

mosra · 2021-06-28T19:50:25Z

src/esp/gfx/DebugLineRender.cpp

+    if (dist1 < radius) {
+      return false;
+    }
+    const float lerpFraction = (radius - dist0) / (dist1 - dist0);


Might want to look at Math::lerpInverted() :)

mosra · 2021-06-28T19:52:44Z

src/esp/gfx/DebugLineRender.cpp

+
+  glDisable(GL_LINE_SMOOTH);  // anti-aliased lines cause artifacts
+
+  glLineWidth(_internalLineWidth);


GL::Renderer::setLineWidth(), but note that line widths larger than 1 are "not core GL" and thus you will only get this on NVidia, which has this only because CAD vendors refuse to update their ancient GL code.

(Yes, yes, I need to add a wide line renderer to Magnum, this is ridiculous.)

FWIW, this feature is working fine on my Macbook with AMD Radeon Pro 5500M.

(Yes, yes, I need to add a wide line renderer to Magnum, this is ridiculous.)

👍

mosra · 2021-06-28T19:54:20Z

src/esp/gfx/DebugLineRender.cpp

+  const float x = _internalLineWidth * 1.2f;
+  constexpr float sqrtOfTwo = 1.4142f;
+  // hard-coding 8 points around a circle
+  const std::vector<Mn::Vector3> offsets = {Mn::Vector3(x, x, 0),


Suggested change

const std::vector<Mn::Vector3> offsets = {Mn::Vector3(x, x, 0),

constexpr Mn::Vector3 offsets[] = {Mn::Vector3(x, x, 0),

No need for an allocation every time. Range-for works with C arrays as well.

mosra · 2021-06-28T19:54:55Z

src/esp/gfx/DebugLineRender.cpp

+  // hard-coding 8 points around a circle
+  const std::vector<Mn::Vector3> offsets = {Mn::Vector3(x, x, 0),
+                                            Mn::Vector3(-x, x, 0),
+                                            Mn::Vector3(x, -x, 0),


Btw., no need for typing that much:

Suggested change

Mn::Vector3(x, -x, 0),

{x, -x, 0},

mosra · 2021-06-28T19:58:11Z

src/esp/gfx/DebugLineRender.cpp

+  // perf todo: do a custom shader constant for opacity instead so we don't have
+  // to touch all the verts


Use Shaders::FlatGL3D with Flag::VertexColor enabled and call setColor({1.0f, 1.0f, 1.0f, opacity) to get this effect.

The VertexColorGL3D shader is there mainly because it's dead-simple and thus suitable for learning purposes.

mosra · 2021-06-28T19:58:49Z

src/esp/gfx/DebugLineRender.cpp

+  }
+
+  // restore to a reasonable default
+  glLineWidth(1.0);


GL::Renderer::setLineWidth() again

mosra · 2021-06-28T20:23:49Z

src/esp/gfx/DebugLineRender.cpp

+void DebugLineRender::drawBox(const Magnum::Vector3& min,
+                              const Magnum::Vector3& max,
+                              const Magnum::Color4& color) {
+  // 4 lines along x axis
+  drawTransformedLine(Mn::Vector3(min.x(), min.y(), min.z()),
+                      Mn::Vector3(max.x(), min.y(), min.z()), color);
+  drawTransformedLine(Mn::Vector3(min.x(), min.y(), max.z()),
+                      Mn::Vector3(max.x(), min.y(), max.z()), color);
+  drawTransformedLine(Mn::Vector3(min.x(), max.y(), min.z()),
+                      Mn::Vector3(max.x(), max.y(), min.z()), color);
+  drawTransformedLine(Mn::Vector3(min.x(), max.y(), max.z()),
+                      Mn::Vector3(max.x(), max.y(), max.z()), color);
+
+  // 4 lines along y axis
+  drawTransformedLine(Mn::Vector3(min.x(), min.y(), min.z()),
+                      Mn::Vector3(min.x(), max.y(), min.z()), color);
+  drawTransformedLine(Mn::Vector3(max.x(), min.y(), min.z()),
+                      Mn::Vector3(max.x(), max.y(), min.z()), color);
+  drawTransformedLine(Mn::Vector3(min.x(), min.y(), max.z()),
+                      Mn::Vector3(min.x(), max.y(), max.z()), color);
+  drawTransformedLine(Mn::Vector3(max.x(), min.y(), max.z()),
+                      Mn::Vector3(max.x(), max.y(), max.z()), color);
+
+  // 4 lines along z axis
+  drawTransformedLine(Mn::Vector3(min.x(), min.y(), min.z()),
+                      Mn::Vector3(min.x(), min.y(), max.z()), color);
+  drawTransformedLine(Mn::Vector3(max.x(), min.y(), min.z()),
+                      Mn::Vector3(max.x(), min.y(), max.z()), color);
+  drawTransformedLine(Mn::Vector3(min.x(), max.y(), min.z()),
+                      Mn::Vector3(min.x(), max.y(), max.z()), color);
+  drawTransformedLine(Mn::Vector3(max.x(), max.y(), min.z()),
+                      Mn::Vector3(max.x(), max.y(), max.z()), color);
+}
+
+void DebugLineRender::drawCircle(const Magnum::Vector3& pos,
+                                 float radius,
+                                 const Magnum::Color4& color,
+                                 int numSegments,
+                                 const Magnum::Vector3& normal) {
+  // https://stackoverflow.com/questions/11132681/what-is-a-formula-to-get-a-vector-perpendicular-to-another-vector
+  auto randomPerpVec = normal.z() < normal.x()
+                           ? Mn::Vector3(normal.y(), -normal.x(), 0)
+                           : Mn::Vector3(0, -normal.z(), normal.y());
+
+  pushTransform(Mn::Matrix4::lookAt(pos, pos + normal, randomPerpVec) *
+                Mn::Matrix4::scaling(Mn::Vector3(radius, radius, 0.f)));
+
+  Mn::Vector3 prevPt;
+  for (int seg = 0; seg <= numSegments; seg++) {
+    Mn::Deg angle = Mn::Deg(360.f * float(seg) / numSegments);
+    Mn::Vector3 pt(Mn::Math::cos(angle), Mn::Math::sin(angle), 0.f);
+    if (seg > 0) {
+      drawTransformedLine(prevPt, pt, color);
+    }
+    prevPt = pt;
+  }
+
+  popTransform();
+}


No need to reinvent anything :)

I was thinking you could have a generic drawPrimitive() utility:

void DebugLineRender::drawPrimitive(const Mn::Trade::MeshData& mesh, const Mn::Matrix4& transformation, const Mn::Color4& color) { /* Turn the mesh into nonindexed lines first */ if(mesh.primitive() != Mn::MeshPrimitive::Lines) { CORRADE_INTERNAL_ASSERT(mesh.primitive() == Mn::MeshPrimitive::LineLoop || mesh.primitive() == Mn::MeshPrimitive::LineStrip); return drawPrimitive(Mn::MeshTools::generateIndices(mesh), transformation, color); } if(mesh.isIndexed()) return drawPrimitive(Mn::MeshTools::duplicate(mesh), transformation, color); /* Steal all the positions */ for(const Mn::Vector3& position: mesh.attribute<Mn::Vector3>(Mn::Trade::MeshAttribute::Position)) arrayAppend(_verts, Cr::InPlaceInit, transformation.transformPoint(position), color); }

and then just reuse whatever existing primitive code (didn't you also have a debug renderer with colored axes? there's Primitives::axis3D() for that too):

void DebugLineRender::drawBox(const Mn::Vector3& min, const Mn::Vector3& max, const Mn::Color4& color) { drawPrimitive(Mn::Primitives::cubeWireframe(), _cachedInputTransform* Mn::Matrix4::translation((max + min)/2.0f)* /* i hope this is correct? */ Mn::Matrix4::scaling((max - min)/2.0f), color); } void DebugLineRender::drawCircle(const Mn::Vector3& pos, float radius, int segments, const Mn::Color4& color, const Mn::Vector3& normal) { ... drawPrimitive(Mn::Primitives::circle3DWireframe(segments), _cachedInputTransform* Mn::Matrix4::lookAt(pos, pos + normal, randomPerpVec)* Mn::Matrix4::scaling(Mn::Vector3(radius, radius, 0.f)), color); }

Add #includes as needed. The Primitives library should be sufficiently lightweight to have negligible perf impact when used this way.

I feel like your suggestion here is more complicated than what I have. From my perspective, a bit of code to create a circle or a box is simple and readable, and users can easily adapt it later to their own shapes (tetrahedrons, helices, etc.).

The code has a lot of extra API calls with long names to massage the mesh data, yes, but the essence of it is a single loop that copies line endpoints from a large gallery of existing primitives, where adding another one (such as a cone for visualizing spotlights) is extremely trivial :)

I commented also because my past experience is that any primitive generation code that wasn't thoroughly tested tended to rot over time, getting increasingly broken during minor refactorings. Since I suppose you won't want to spend time writing tests ensuring the mesh is generated correctly, this would delegate that responsibility to the engine.

For now, rather than drag out the discussion, let's agree to disagree. :)

I'll add a comment in the file pointing to this discussion, in case folks in the future want to go this route.

mosra · 2021-06-28T20:37:49Z

src/esp/gfx/DebugLineRender.cpp

+  // 1.2 is hand-tuned for Nvidia hardware to be just small enough so we don't
+  // see gaps between the individual offset lines.
+  const float x = _internalLineWidth * 1.2f;
+  constexpr float sqrtOfTwo = 1.4142f;


Mn::Constants::sqrt2()

mosra · 2021-06-28T20:45:26Z

src/esp/gfx/DebugLineRender.cpp

+}
+
+void DebugLineRender::drawPathWithEndpointCircles(
+    const std::vector<Mn::Vector3>& points,


FYI you could take an Containers::ArrayView<const Vector3>, and then if you #include <Corrade/Containers/ArrayViewStl.h>, the std::vector is implicitly convertible to it, but it allows you to use any other contiguous container (std::array, a C array...) as well.

Could be worth it if you're often submitting paths with known sizes and can thus have e.g. Vector3 path[5] { ... } instead of populating a vector, if it's always a dynamic vector then probably not.

I support all efforts to reduce our per-frame dynamic allocations!

Hmmmm, in fact I was calling this in one test with a fixed list, but I hit this:

lineRender->drawPathWithEndpointCircles( {{0.f, 0.f, 0.f}, {0.1f, 0.f, 0.f}}, radius, color);

viewer.cpp:1156:55: error: cannot convert ‘<brace-enclosed initializer list>’ to ‘Corrade::Containers::ArrayView<const Magnum::Math::Vector3<float> >’

C++ is unintuitive. Lengthy answer why is it like that -- see the yellow box titled "Conversion from std::initializer_list" there.

IOW, the API is designed to avoid very common use-after-free issues, which unfortunately makes this one use case annoying. What I commonly do on the engine side is adding a std::initializer_list overload for every function that takes an ArrayView. Like

void DebugLineRender::drawPathWithEndpointCircles(const Cr::Containers::ArrayView<const Mn::Vector3> points, ...) { ... } void DebugLineRender::drawPathWithEndpointCircles(const std::initializer_list<Mn::Vector3> points, ...) { drawPathWithEndpointCircles(Containers::arrayView(points), ...); }

Without an overload, in the case above you'd have to do a bit more typing:

lineRender->drawPathWithEndpointCircles( Containers::arrayView<Vector3>({{0.f, 0.f, 0.f}, {0.1f, 0.f, 0.f}}), radius, color);

Ok. I'm not too worried about the ugly callsite I gave above; it was just some temp testing code. I don't think the initializer_list version is necessary.

I've switched to ArrayView and also added a wrapper version that takes a std::vector; this was needed for the Python binding.

…Python binding

aclegg3

LGTM. One comment on the bindings.

aclegg3 · 2021-07-01T17:49:19Z

src/esp/bindings/GfxBindings.cpp

+      .def("draw_box", &DebugLineRender::drawBox,
+           R"(Draw a box in world-space or local-space (see pushTransform).)")


No color for box bindings?

draw_box takes two Vector3 and a color. This binding here just takes all the C++ function parameters as-is without explicitly declaring them all here.

I guess this just makes it harder for python only user seeing the doc-string API reference to know the parameters. Also, does implicit conversion allow keyword (named) arguments?

eundersander added 4 commits June 28, 2021 11:31

add DebugLineRender util for thick-line-rendering

473bb7f

integrate DebugLineRender into Simulator

ce39983

Python bindings for DebugLineRender

3cb7729

update replay_tutorial.py: debug line rendering; brighter lights

7e1f726

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 28, 2021

eundersander commented Jun 28, 2021

View reviewed changes

aclegg3 reviewed Jun 28, 2021

View reviewed changes

eundersander requested a review from bigbike June 28, 2021 19:25

eundersander added 2 commits June 28, 2021 12:26

fix for logging include

2204df6

polish

8dbd74e

mosra reviewed Jun 28, 2021

View reviewed changes

eundersander added 4 commits June 28, 2021 16:40

compile fix for JS build; fix deprecation warning

8efc781

switch to FlatGL; switch to ArrayView for points list; other polish

fc3540e

version of drawPathWithEndpointCircles that takes a std::vector, for …

6ba198a

…Python binding

update comment

2dfbdbb

aclegg3 approved these changes Jul 1, 2021

View reviewed changes

eundersander merged commit 6345a4c into facebookresearch:master Jul 1, 2021

eundersander deleted the eundersander/add_debug_line_render branch July 1, 2021 20:37

	R"(Draw a line segment in world-space or local-space (see pushTransform).)")
	R"(Draw a line segment in world-space or local-space (see pushTransform) with interpolated color.)")

	* submitting opaque scene geomtry.
	* submitting opaque scene geometry.

	return Mn::Color4(src.r(), src.g(), src.b(), std::pow(src.a(), exponent));
	return {src.rgb(), std::pow(src.a(), exponent)};


		glDisable(GL_LINE_SMOOTH); // anti-aliased lines cause artifacts

		glLineWidth(_internalLineWidth);

	const std::vector<Mn::Vector3> offsets = {Mn::Vector3(x, x, 0),
	constexpr Mn::Vector3 offsets[] = {Mn::Vector3(x, x, 0),

		// perf todo: do a custom shader constant for opacity instead so we don't have
		// to touch all the verts

		.def("draw_box", &DebugLineRender::drawBox,
		R"(Draw a box in world-space or local-space (see pushTransform).)")

Add DebugLineRender utility #1349

Add DebugLineRender utility #1349

Conversation

eundersander commented Jun 28, 2021

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

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

aclegg3 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eundersander Jun 29, 2021 • edited

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 Jun 28, 2021 • edited

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

aclegg3 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

eundersander Jun 29, 2021 •

edited

mosra Jun 28, 2021 •

edited