Add a left margin to TimeGraph #4488

pierricgimmig · 2022-11-25T01:10:21Z

It is assumed at various places that the thread tracks are positioned at 0 horizontally. To uncover those assumptions, and for easier future layout modifications, we add a left margin to time graph layout. It is set to zero, so there is no functional change in this PR. To visualize the effect, the "Left Margin" property has been exposed to the "devmode" layout UI.

florian-kuebler · 2022-11-25T10:30:42Z

Reassigning to @reichlfl. Code-wise it looks good. But to be honest, I lost a bit of the overview of the ui layouting part. @karupayun and @reichlfl are more into that.

src/OrbitGl/CallstackThreadBar.cpp

karupayun

Thank you very much Pierric! This PR was really needed :)

I left some comments and I will work in parallel to fix the GetPixelNumber bug

src/OrbitGl/CallstackThreadBar.cpp

src/OrbitGl/TimeGraph.cpp

src/OrbitGl/include/OrbitGl/StaticTimeGraphLayout.h

src/OrbitGl/TimeGraph.cpp

src/OrbitGl/include/OrbitGl/CaptureViewElement.h

src/OrbitGl/CaptureViewElement.cpp

pierricgimmig · 2022-11-28T15:56:35Z

Thanks for your comments @karupayun and @reichlfl , I'll get back to this this week. Feel free to implement some of the fixes mentioned in the comments yourself btw. Thanks!

orbitprofiler-bot

Thanks for your change. Please address the suggested formatting changes.

orbitprofiler-bot · 2022-12-04T18:45:22Z

src/OrbitGl/TimelineUi.cpp

-  // Check that the label is visible or partially visible.
-  if (ClosedInterval<float> label_x_interval{world_x, world_x + label_width};
-      !label_x_interval.Intersects(ClosedInterval<float>{GetPos()[0], GetPos()[0] + GetWidth()})) {
+


Suggested change

orbitprofiler-bot

Thanks for your change. Please address the suggested formatting changes.

orbitprofiler-bot · 2022-12-04T19:06:05Z

src/OrbitGl/TimelineUi.cpp

-  // Check that the label is visible or partially visible.
-  if (ClosedInterval<float> label_x_interval{world_x, world_x + label_width};
-      !label_x_interval.Intersects(ClosedInterval<float>{GetPos()[0], GetPos()[0] + GetWidth()})) {
+


Suggested change

pierricgimmig · 2022-12-04T19:25:59Z

Hi @reichlfl , hi @karupayun ,

I have implemented the suggested changes. The original idea was to make it easier to change the layout, specifically, I'm interested in drawing elements at the left of the thread tracks. With this PR, it is now the responsibility of capture view elements to make sure they draw within their allocated space, except for the timeline ui. In my view, this is better than the existing cover-up of the right margin because it forces us to draw strictly what is needed, and it frees up the space on both the left and right margin to draw other things easily, without z-value tricks. Consequently, I removed the right-margin rectangle for everything except the timeline ui, which I'm leaving completely unchanged. I had to add a left margin rectangle, but only for the height of the timeline, leaving the area below available for drawing.

I changed the HorizontalClamp method to ClampToWorldExtentsX, but I left it in CaptureViewElement.h because it's dealing with private members of this class, and that any draw element could eventually use it, I don't see how this hurts.

This PR also fixes several assumptions/bugs in UI tests.

Here it is in action, with dummy margin values. Please note that this PR doesn't change the actual margin values, so the left margin will be 0, and the right margin 10.

margins.mp4

orbitprofiler-bot

Thanks for your change. Please address the suggested formatting changes.

src/OrbitGl/TimelineUi.cpp

karupayun

Hey Pierric. I really like the idea of not depending of the z-layers + margins to assure containers are not drawing outside of their respective area.

However clamping doesn't work. It works for lines parallel to the axis and rectangles but stop working for everything else. In our case, as far as I know, it only doesn't work for the triangles in the GpuDebugMarker, see screenshots of how they change their shape and the text doesn't fit anymore.

I started a prototype for properly solving it in the CPU:
1- Each container tells the Batcher their pos + size.
2- The Batcher taking into account the position + size of the ancestors:

If all points are inside, they send the primitive as it is
If not, it intersects the shape with the drawing zone of the container doing segment to segment intersection (there are some details that we can discuss if you are interested)
Finally it draws the new shape (that potentially might have more points).
Assuming that most primitives will fit in the container, the performance regression shouldn't be a big deal.

It's a bunch of work that will take a bit of time and I don't have it. We discussed with @reichlfl and we agreed of properly solve the shape intersection issue in the Gpu (that actually makes more sense). Florian will try to do some experiments to see if we have performance regression there, but the idea is setting the position + size of the container into account and let the Gpu do the work for us.

If this idea doesn't work we can work in the Cpu solution. Meanwhile, I would say setting a left-margin and if you need to draw on top of the margin, I would say creating a new CaptureViewElement who draws on top (what do you want to draw there?)

What do you think? Do you have another suggestion?

pierricgimmig · 2022-12-05T20:41:07Z

What do you think? Do you have another suggestion?

It seems the only breaking part of this PR is for the GpuDebugMarkers. The fix is fairly easy to implement, essentially modifying the clipping code in TimerTrack. Let's say that was done, would this PR get approved?

karupayun · 2022-12-05T21:28:51Z

What do you think? Do you have another suggestion?

It seems the only breaking part of this PR is for the GpuDebugMarkers. The fix is fairly easy to implement, essentially modifying the clipping code in TimerTrack. Let's say that was done, would this PR get approved?

Are you also planning to erase the top margin? Because if so, I don't understand how that can be solved modifying the clipping code in TimerTrack. Some Triangles can have 4 or even 5 vertices after clipping.

I don't find it consistently and we will have bugs in the feature (I guess for example that we are also breaking the white arrow between thread state dependencies).

I would say that if you want that, we should erase also the generic AddLine and have 2 methods AddHorizontalLine and AddVerticalLine, but I don't think that this is the right way, interested in @reichlfl opinion, since tomorrow I start vacation until EOY and I won't be looking for this.

PS: Both the green-overlay and the iterators are being drawn on top of the margins now.

reichlfl · 2022-12-06T16:46:28Z

I have to admit that I'm not sure about this change. I agree with the intent, and I definitely like that we do not assume all tracks are positioned at 0 and span the whole width. So I would definitely merge these parts. I also think it's a good idea to provide facilities in CaptureViewElement to restrict their contents to their own body, and use this where possible.

I don't think that a CaptureViewElement should have to ensure this by manually clipping triangles though. This feels like too much effort put onto the implementor of UI elements. It would be doable for GPU tracks, but it already gets complicated if you'd want to consider correct clipping on top/bottom as well. I think it should be responsibility of the framework to ensure drawing of element content is restricted to their body...

For me, the ideal state which would make a lot of calculations obsolete and would allow more flexible layouting would be:

Element positions are always relative, so you don't need to care about global positions
Elements do their best to restrict drawing to their body, but it is ultimately enforced by the renderer.

I'm experimenting with a solution for 2) by restricting the draw area with the stencil buffer before each element. I'll let you know if this comes at an acceptable performance hit.

There's also other non-breaking, but weird implications to these margins. As Pablo pointed out, the green selection is still visible in the margin, but not behind the cover-up rectangles in the timeline UI...

Meanwhile, I would actually suggest the following:

Stick with the cover-up rectangles for the left / right margins for the full height and hope to get rid of it in the future
Keep your changes regarding position and clipping - they don't break stuff, and if we can restrict the rendering to the element body on the CPU already, all the better
Keep the GPU track as-is, it'll be covered up by the rectangles in the margins.

WDYT?

reichlfl · 2022-12-06T16:07:21Z

src/OrbitGl/include/OrbitGl/CaptureViewElement.h

@@ -50,6 +50,11 @@ class CaptureViewElement : public Pickable, public AccessibleInterfaceProvider {
  [[nodiscard]] Vec2 GetSize() const { return Vec2(GetWidth(), GetHeight()); }
  [[nodiscard]] virtual bool ShouldBeRendered() const { return GetVisible(); }

+  // Clamp the passed in x value to the horizontal world extents.
+  [[nodiscard]] float ClampToWorldExtentsX(float pos_x) const {


Nit: I find this name still confusing, it sounds like you clamp to the extents of the world. Maybe ClampXToInsideElement or something?

reichlfl · 2022-12-06T16:12:03Z

src/OrbitGl/SliderTest.cpp

  tester.SimulatePreRender(slider.get());

+  // In CreateTestSlider, we have called SetWidth() which sets "update_primitives_requested_" to
+  // true. Simulate a draw loop to reinitialize the draw flags before testing their values below.
+  tester.SimulateDrawLoop(slider.get(), true, true);


I think with this line you don't need the SimulatePreRender before.

It is assumed at various places that the thread tracks are positioned at 0 horizontally. To uncover those assumptions, and for easier future layout modifications, we add a left margin to time graph layout. It is set to zero, so there is no functional change in this change. To visualize the effect, the "Left Margin" has been exposed to the "devmode" layout UI.

The horizontal slider assumed a layout that is not necessarily valid, remove that assumption and rely on the actual position and width of the slider. Also, zooming tests when left margin is not 0.

Left margin now uses the same masking trick as the right margin.

pierricgimmig · 2022-12-07T16:29:10Z

@reichlfl , @karupayun , thanks for your comments. I removed the ClampToWorldExtent in favor of using the masking quad as is done on the right margin. PTAL, thanks.

reichlfl

Thanks for the changes! PTAL at the open comments and fix if needed. Otherwise this LGTM, sorry if this was a lot of back and forth - layouting is still more complicated than it needs to be I'm afraid. I'll loop you in on any changes I do to restricting the drawing area to individual elements, maybe we can then remove the margin boxes in the future.

reichlfl · 2022-12-08T08:36:30Z

src/OrbitGl/TimeGraph.cpp

-  primitive_assembler.AddBox(box, GlCanvas::kZValueMargin, GlCanvas::kBackgroundColor);
+  // Right margin mask for Timeline.
+  float right_margin_width = GetRightMargin();
+  float right_margin_height = GetHeight() - timeline_ui_->GetHeight();


Shouldn't this be GetHeight()? The margin spans across the timeline and is positioned at y==0, so why subtract the timeline height?

Looking at the code above - should this be GetHeight() - horizontal_slider_->GetHeight()? The margin ends above the slider.

By the way, what's the reasoning behind the different behaviors of left and right margin (left one goes all the way to the bottom, right one stops above the slider)? Wouldn't it be more consistent to have the horizontal slider also stop at the margin?

Thanks for the review @reichlfl . The reasoning is that I found it weird to have the horizontal slider not touch the horizontal slider, feel free to modify. I've done enough work on this PR :-)

pierricgimmig requested review from karupayun and florian-kuebler November 25, 2022 01:10

pierricgimmig force-pushed the feature/left_margin branch from f7cc12e to 2828b8e Compare November 25, 2022 04:05

florian-kuebler requested review from reichlfl and removed request for florian-kuebler November 25, 2022 10:29

reichlfl suggested changes Nov 25, 2022

View reviewed changes

src/OrbitGl/CallstackThreadBar.cpp Outdated Show resolved Hide resolved

karupayun reviewed Nov 25, 2022

View reviewed changes

pierricgimmig force-pushed the feature/left_margin branch 3 times, most recently from 22e5d5b to 2e708d2 Compare December 4, 2022 18:43

orbitprofiler-bot reviewed Dec 4, 2022

View reviewed changes

pierricgimmig force-pushed the feature/left_margin branch from 2e708d2 to c9ca16a Compare December 4, 2022 19:04

orbitprofiler-bot reviewed Dec 4, 2022

View reviewed changes

src/OrbitGl/TimelineUi.cpp Outdated Show resolved Hide resolved

pierricgimmig requested review from reichlfl, karupayun and orbitprofiler-bot December 4, 2022 22:12

karupayun reviewed Dec 5, 2022

View reviewed changes

reichlfl reviewed Dec 6, 2022

View reviewed changes

pierricgimmig and others added 5 commits December 7, 2022 07:18

Remove HorizontalSlider::Width() incorrect override

5064196

The horizontal slider assumed a layout that is not necessarily valid, remove that assumption and rely on the actual position and width of the slider. Also, zooming tests when left margin is not 0.

SliderTest fixes

8e0bb16

Make tracks responsible for drawing inside their allocated space

af72d46

Only apply margin mask to timeline

d2e299d

pierricgimmig added 4 commits December 7, 2022 07:18

Clang format

c303f1c

Fix templated baseclass compilation error

ca1531f

Fix templated baseclass compilation error

5bab807

Remove ClampToWorldExtentsX

e551123

Left margin now uses the same masking trick as the right margin.

pierricgimmig requested review from reichlfl and karupayun December 7, 2022 15:43

pierricgimmig force-pushed the feature/left_margin branch from 5e67719 to e551123 Compare December 7, 2022 15:51

reichlfl approved these changes Dec 8, 2022

View reviewed changes

pierricgimmig merged commit becea4e into google:main Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a left margin to TimeGraph #4488

Add a left margin to TimeGraph #4488

pierricgimmig commented Nov 25, 2022

florian-kuebler commented Nov 25, 2022

karupayun left a comment

pierricgimmig commented Nov 28, 2022

orbitprofiler-bot left a comment

orbitprofiler-bot Dec 4, 2022

orbitprofiler-bot left a comment

orbitprofiler-bot Dec 4, 2022

pierricgimmig commented Dec 4, 2022 •

edited

Loading

orbitprofiler-bot left a comment

karupayun left a comment •

edited

Loading

pierricgimmig commented Dec 5, 2022

karupayun commented Dec 5, 2022

reichlfl commented Dec 6, 2022 •

edited

Loading

reichlfl Dec 6, 2022

reichlfl Dec 6, 2022

pierricgimmig commented Dec 7, 2022

reichlfl left a comment

reichlfl Dec 8, 2022

reichlfl Dec 8, 2022

pierricgimmig Dec 8, 2022

Add a left margin to TimeGraph #4488

Add a left margin to TimeGraph #4488

Conversation

pierricgimmig commented Nov 25, 2022

florian-kuebler commented Nov 25, 2022

karupayun left a comment

Choose a reason for hiding this comment

pierricgimmig commented Nov 28, 2022

orbitprofiler-bot left a comment

Choose a reason for hiding this comment

orbitprofiler-bot Dec 4, 2022

Choose a reason for hiding this comment

orbitprofiler-bot left a comment

Choose a reason for hiding this comment

orbitprofiler-bot Dec 4, 2022

Choose a reason for hiding this comment

pierricgimmig commented Dec 4, 2022 • edited Loading

orbitprofiler-bot left a comment

Choose a reason for hiding this comment

karupayun left a comment • edited Loading

Choose a reason for hiding this comment

pierricgimmig commented Dec 5, 2022

karupayun commented Dec 5, 2022

reichlfl commented Dec 6, 2022 • edited Loading

reichlfl Dec 6, 2022

Choose a reason for hiding this comment

reichlfl Dec 6, 2022

Choose a reason for hiding this comment

pierricgimmig commented Dec 7, 2022

reichlfl left a comment

Choose a reason for hiding this comment

reichlfl Dec 8, 2022

Choose a reason for hiding this comment

reichlfl Dec 8, 2022

Choose a reason for hiding this comment

pierricgimmig Dec 8, 2022

Choose a reason for hiding this comment

pierricgimmig commented Dec 4, 2022 •

edited

Loading

karupayun left a comment •

edited

Loading

reichlfl commented Dec 6, 2022 •

edited

Loading