Fixes #3761, #2886 - Draw and Layout performance issues

[tig]

Fixes

• Fixes #3761
• Fixes #2886
• Fixes #3780
• Fixes #3485

Related

• #2307

Proposed Changes/Todos

• [ ] Decouple layout from draw
• [ ] Enforce that Clear rarely happens
• [ ] Ensure drivers minimize full repaints
• [ ] Minimize when Draw/OnDrawContent are called
• [ ] Potentially add non-rectangular clip regions - #3413

Pull Request checklist:

• [ ] I've named my PR in the form of "Fixes #issue. Terse description."
• [ ] My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• [ ] My code follows the Terminal.Gui library design guidelines
• [ ] I ran dotnet test before commit
• [ ] I have made corresponding changes to the API documentation (using /// style comments)
• [ ] My changes generate no new warnings
• [ ] I have checked my code and corrected any poor grammar or misspellings
• [ ] I conducted basic QA to assure all features are working

[tig]

@BDisp, @tznind, @dodexahedron or anyone else who'd like to comment...

As I've worked this PR, I've learned a bunch of things, and gotten clarity on other issues that have bugged me for a long time.

Before I go further, I want to share my thoughts in writing. This will solidify my thinking and enable y'all to challenge and debate it.

Some of the changes I've made, and want to continue to make are significant. While they don't fundamentally change the TG programming model, they do require changes to how devs will develop against it. Some will make porting from v1 more challenging.

Layout vs Draw

First, TG has always been confused about the relationship between Layout and Draw. This confusion got worse and worse over time. The addition of View.AutoSize made it even worse. As did more sophisticated use-cases including overlapped Views, multi-threading, and Adornments. The state of v2_develop is horrific in this regard. Hence #3761 etc... that lead to this PR.

The codebase is full of convoluted code that attempts to ensure things get laid out then drawn. Property _set methods call a combination of SetNeedsDisplay, SetLayoutNeeded, and LayoutSubviews. The default OnDrawContent forces layout because it can't assume layout has already happened. The mainloop uses brute-force-overkill to force layouts and draws and ends up calling Consoledriver.Refresh way too frequently. Plus View.Clear is called constantly. Some Views (TabView) re-layout elements DURING draw!

We have to fix this at a fundamental level or the library will continue to get bogged down.

Assertions:

• Layout and Draw must be decoupled - Layout code should not call draw code. Draw code should not call layout code.
• Layout happens first; then Draw - This sounds obvious, but there are multiple places where the library or built-in Views either blindly do the opposite or attempt to do layout mid-draw.

Asynchronous Layout vs Explicit/Synchronous

Way back, the only place that called SetRelativeLayout and LayoutSubviews was the mainloop. MG's design for gui-cswas these methods were to be called each mainloop iteration. They were internal for this reason. Overtime, though, folks started using them more and more from within View implementations or app code.

The names of these methods leads to confusion as well.

Assertion:

• Layout is Asynchronous - In the vast majority of cases developers should not call SetRelativeLayout or LayoutSuviews directly, but should just signal layout is needed with SetLayoutNeeded(). We will introduce a new API (which is really just a refactor of the currently private LayoutSubview(): public bool Layout (Size contentSize) that first calls SetRelativeLayout and then LayoutSubviews. Devs can use this in the rare cases where an immediate layout is required.

Great, but what about cases where a dev DOES want immediate layout? Should it be implicit (as it is today) or explicit?

Option 1

• Frame, X, Y, Width, and Height are non-deterministic if IsLayoutNeeded is true - This is a BIG one. It means this code will fail on the 2nd assert:

  View v = new ();
  Assert.Equal (0, v.Frame.Height);
  v.Height = 1;
  Assert.Equal (1, v.Frame.Height);

This will not fail:

  View v = new ();
  Assert.Equal (0, v.Frame.Height);
  v.Height = 1;
  Assert.Equal (0, v.Frame.Height);
  v.Layout ();  
  Assert.Equal (1, v.Frame.Height);

Option 2

• Frame, X, Y, Width, and Height are non-deterministic if IsLayoutNeeded is true
• Setting X, Y, Width, and Height to a Pos/DimAbsolute will call Layout() (causing IsLayoutNeeded to be false) IFF the other 3 are also Pos/DimAbsolute.
• Setting Frame, will cause will call Layout()(causingIsLayoutNeededto befalse`).

The downside of this is that devs may be confused:

  View v = new ();
  Assert.Equal (0, v.Frame.Height);
  v.Height = 1;
  Assert.False (v.IsLayoutNeeded());
  Assert.Equal (1, v.Frame.Height); // works because `1` is `Dim.Absoulte(1)`

  v.Width = Dim.Percent(50);
  Assert.True (v.IsLayoutNeeded());
  Assert.Equal (0, v.Frame.Width); // still 0

The reality is the above code is really only valid in UNIT TESTS. No dev will ever do this IRL.

IOW, if we don't call SetLayout in the Pos/DimAbsoulte cases, all unit tests will need to explicitly call Layout which is a PITA.

In this PR I started with Option 1. It felt pure. But it means I have to touch 100s of Unit tests that simply are using absolute values and add calls to v.Layout(). So now I'm going with Option 2.

.... More later. This is enough for now...

[BDisp]

As I remember the layout was always done before drawn. When the drawn was called alone and IsLayoutNeeded() == true the layout was being performed first. I also agree with the option 2. The #3761 really needs to be fixed. Good work.

[tznind]

Awesome!

This will definitely help with our flicker issue and bring some structure to layout - which has been a bit unstructured and opaque.

Regarding redraw - i think we can also eliminate redraws of identical screen content. I did this in the sixel branch to work around the constant refreshes and it seemed to work. Basically on screen redraw you record the final string sent to console and if it's the same you bail. This could be a 'last line of defence' against this kind of thing.

On Sat, 19 Oct 2024, 00:29 BDisp, @.***> wrote:

As I remember the layout was always done before drawn. When the drawn was called alone and IsLayoutNeeded() == true the layout was being performed first. I also agree with the option 2. The #3761 https://github.com/gui-cs/Terminal.Gui/issues/3761 really needs to be fixed. Good work.

— Reply to this email directly, view it on GitHub https://github.com/gui-cs/Terminal.Gui/pull/3798#issuecomment-2423367846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO3C5DRFANN3IZSQDEWGCTZ4GKVBAVCNFSM6AAAAABQAGJGOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRTGM3DOOBUGY . You are receiving this because you were mentioned.Message ID: @.***>

[tig]

This is not really ready to merge, but it def needs review!

I've changed a TON of the draw and layout API surface area here. Not just names but semantics.

The new unit tests and more sophisticated Scenario test code (esp AllViewsTester) show this is all in the right direction.

But I'm sure I've messed things up, over engineered, or changed things that are hard to understand.

I need y'all to review this carefully and comment. thanks.

[tznind]

I am having a look at TabView

The reason that it does layout in draw is because of https://github.com/gui-cs/Terminal.Gui/pull/2980

Prior to this PR there were 2 views only (content view and tab row view). The row view drew all the characters direct from the driver e.g. scroll indicators, tabs etc.

After #2980 these all became views (_leftScrollIndicator etc). But the positioning stayed where it was:

// draw scroll indicators

// if there are more tabs to the left not visible
if (host.TabScrollOffset > 0) {
	Move (0, y);

	// indicate that
	Driver.AddRune (CM.Glyphs.LeftArrow);

Became

// draw scroll indicators
// if there are more tabs to the left not visible
	if (_host.TabScrollOffset > 0) {
		_leftScrollIndicator.X = 0;
		_leftScrollIndicator.Y = y;

		// indicate that
		_leftScrollIndicator.Visible = true;
		// Ensures this is clicked instead of the first tab
		BringSubviewToFront (_leftScrollIndicator);
		_leftScrollIndicator.Draw ();
	} else {
		_leftScrollIndicator.Visible = false;
	}

I haven't worked with it after the change but will have a bash at creating a layout method where this happens instead.

[BDisp]

I'm also having a look at this, since I implemented the new Border adornment. The drawing issue I already fixed and I'm only dealing with key navigation, right now.

[tznind]

Ok awesome I will hold off then - no point in duplicating 👍 thanks!

[tig]

IMO TabRowView should just be a SuperView for a set of Views that have their borders set.

We should enable Border to support different line styles for each side by making each side be a Line that's a subview of Border.

We have enough examples of adornments with subviews working well (and the draw perf pr makes it even better) to have confidence that Border can finally get rid of all the custom rendering and just use subviews.

Even better would be each Tab to just be a View with one side having a thickness of 3. We'd need to have Border support the ability to set the start offset of the title.

There should be no need for TabView to use LineCanvas directly.

[BDisp]

@tig I think this code isn't totally correct because it's avoiding the first view of the indicies to get focus if it contains the _previouslyFocused. This is avoiding the tab getting focus if a previously subview had the focus before.

    /// <summary>
    ///     INTERNAL API to restore focus to the subview that had focus before this view lost focus.
    /// </summary>
    /// <returns>
    ///     Returns true if focus was restored to a subview, false otherwise.
    /// </returns>
    internal bool RestoreFocus ()
    {
        View [] indicies = GetFocusChain (NavigationDirection.Forward, null);

        if (Focused is null && _previouslyFocused is { } && indicies.Contains (_previouslyFocused))
        {
            if (_previouslyFocused.SetFocus ())
            {
                return true;
            }

            _previouslyFocused = null;
        }

        return false;
    }

[tznind]

IMO TabRowView should just be a SuperView for a set of Views that have their borders set.

Yes this makes sense. And is why tab view now uses subviews. Wasn't suggesting trying to go back to manual draw just that the reason for all the draw layout was the introduction of subviews.

Current code is half way through a journey that is all

[BDisp]

IMO TabRowView should just be a SuperView for a set of Views that have their borders set.

Yes this makes sense. And is why tab view now uses subviews. Wasn't suggesting trying to go back to manual draw just that the reason for all the draw layout was the introduction of subviews.

Current code is half way through a journey that is all

@tznind can you then please go forward with this approach?

[tig]

We now have pretty sophisticated benchmarking and demo support in UICatalog:

> UICatalog.exe --help

Description:
  A comprehensive sample library for Terminal.Gui

Usage:
  UICatalog [<scenario>] [options]

Arguments:
  The name of the scenario to run. [default: none]
  
Options:
  -b, --benchmark                                                 Enables benchmarking.
  -f, --file <file>                                               The file to save benchmark results to. If not specified with --benchmark, the results will be  displayed in a TableView.
  -d, --driver <CursesDriver|FakeDriver|NetDriver|WindowsDriver>  The ConsoleDriver to use.
  --version                                                       Show version information
  -?, -h, --help                                                  Show help and usage information

Scenario now has an API for providing "Demo Keys":

    public virtual List<Key> GetDemoKeyStrokes () => new List<Key> ();

For example, CharMap does this:

    public override List<Key> GetDemoKeyStrokes ()
    {
        var keys = new List<Key> ();

        for (int i = 0; i < 200; i++)
        {
            keys.Add (Key.CursorDown);
        }

        // Category table
        keys.Add (Key.Tab.WithShift);

        // Block elements
        keys.Add (Key.B);
        keys.Add (Key.L);

        keys.Add (Key.Tab);

        for (int i = 0; i < 200; i++)
        {
            keys.Add (Key.CursorLeft);
        }
        return keys;
    }

So,

[BDisp]

@tig I only have one test failing public void ProcessKey_Down_Up_Right_Left_Home_End_PageDown_PageUp because the navigation is forcing using the RestoreFocus method and thus avoid the SelectedTab being focused and always focus the _previouslyFocused. I understand the use of this method when called explicitly, like the unit tests does, but I don't understand why calling it in a normal navigation, unless in a TabGroup. Can you help me on this, please.

[tig]

@tig I only have one test failing public void ProcessKey_Down_Up_Right_Left_Home_End_PageDown_PageUp because the navigation is forcing using the RestoreFocus method and thus avoid the SelectedTab being focused and always focus the _previouslyFocused. I understand the use of this method when called explicitly, like the unit tests does, but I don't understand why calling it in a normal navigation, unless in a TabGroup. Can you help me on this, please.

Not entirely sure what you mean. Maybe submit your work as a PR against this PR and I can take a look?

[BDisp]

Done in https://github.com/tig/Terminal.Gui/pull/41. The only way to focus the selected tab is if the view container CanFocus is false or if a view is currently selected in the container and pressing cursor up.

[tig]

I renamed "SetNeedsDisplay" to "SetNeedsDraw". Sorry, not sorry.

[tig]

To help visually track down situations where things are getting redrawn when they shouldn't be, I've added ViewDiagnostics.DrawIndicator:

To illustrate the usefulness of this, here's the Navigation scenario shown in the GIF above, with the hack enabled that causes Overalpped Views to be re-drawn constantly so that they appear correctly.

FWIW, the only "right" way to make overlapped views draw efficiently will be to implement non-rectangular clip regions.

For now this hack is enabled in View.DrawSubviews, and it only comes into play in cases like the Navigation Sceanario where Overlapped views are used.

    public void DrawSubviews ()
    {
        if (_subviews is null || !SubViewNeedsDraw)
        {
            return;
        }

#if HACK_DRAW_OVERLAPPED
        IEnumerable<View> subviewsNeedingDraw = _subviews.Where (view => (view.Visible && (view.NeedsDraw || view.SubViewNeedsDraw))
                                                                      || view.Arrangement.HasFlag (ViewArrangement.Overlapped));
#else
        IEnumerable<View> subviewsNeedingDraw = _subviews.Where (view => (view.Visible && (view.NeedsDraw || view.SubViewNeedsDraw)));
#endif

        foreach (View view in subviewsNeedingDraw)
        {
#if HACK_DRAW_OVERLAPPED
            if (view.Arrangement.HasFlag (ViewArrangement.Overlapped))
            {

                view.SetNeedsDraw ();
            }
#endif
            view.Draw ();

        }
    }

[tig]

I've started implementing

• #3413

So far:

• [x] Created a port of System.Drawing.Region that does not use any external libs and only supports ints.
• [x] Ported a bunch of unit tests for Region from System.Drawing.RegionTests.
• [x] Changed ConsoleDriver.Clip to be Region instead of Rectangle
• [x] Hacked View and unit tests to work with this, but there's currently no support for anything but the Region containing a single rect.

Next:

• [x] Change the View drawing APIs DrawContent (Rectangle viewport) -> DrawContent (Region clipRegion)
• [ ] Refactor View.SetClip etc...
• [x] Use CharMap as a test-case of a view that smartly only tries to draw within the clip region.
• [ ] Figure out how to make View.DrawSubviews correctly draw overlapped views, leveraging the new clipRegion.
• [ ] Add unit tests to ensure it all works

[tig]

This vid demonstrates non-rectangular clip regions doing their magic!

[dodexahedron]

Loving the work on this. 👍

Planning to take a good look at the code later today.

[tig]

The current state is pretty good with the exception of:

• Shadows (actually Margin in general) are broken
• ScrollView is broken (and I won't fix... we have a new one coming)
• TabView is broken. Bdisp has a pr i haven't had a chance to look at.
• Attribute setting is brute force. Need to really clean that up because Color comparison is a heavy op.

I have not invested as much energy in unit tests as I've liked.

[tig]

This is how this all works (from drawing.md):

---

Drawing occurs each MainLoop Iteration

The @Terminal.Gui.Application MainLoop will iterate over all Views in the view hierarchy looking for views have their @Terminal.Gui.View.NeedsDraw property set. The @Terminal.Gui.View.Draw method will be called which, in turn:

1. Sets the clip to the view's Frame.
2. Draws the @Terminal.Gui.View.Border and @Terminal.Gui.View.Padding (but NOT the Margin).
3. Sets the clip to the view's Viewport.
4. Sets the Normal color scheme.
5. Calls Draw on any @Terminal.Gui.View.Subviews.
6. Draws @Terminal.Gui.View.Text.
7. Draws any non-text content (the base View does nothing.)
8. Sets the clip back to the view's Frame.
9. Draws @Terminal.Gui.View.LineCanvas (which may have been added to by any of the steps above).
10. Draws the @Terminal.Gui.View.Border and @Terminal.Gui.View.Padding Subviews (just the subviews). (but NOT the Margin).
11. The Clip at this point excludes all Subviews NOT INCLUDING their Margins. This clip is cached so @Terminal.Gui.View.Margin can be rendered later.
12. DrawComplete is raised.
13. The current View's Frame NOT INCLUDING the Margin is excluded from the current Clip region.

Most of the steps above can be overridden by developers using the standard Terminal.Gui cancellable event pattern. For example, the base @Terminal.Gui.View always clears the viewport. To override this, a subclass can override @Terminal.Gui.View.OnClearingViewport to simply return true. Or, a user of View can subscribe to the @Terminal.Gui.View.ClearingViewport event and set the Cancel argument to true.

Then, after the above steps have completed, the Mainloop will iterate through all views in the view hierarchy again, this time calling Draw on any @Terminal.Gui.View.Margin objects, using the cached Clip region mentioned above. This enables Margin to be transparent.

Declaring that drawing is needed

If a View need to redraw because something changed within it's Content Area it can call @Terminal.Gui.View.SetNeedsDraw. If a View needs to be redrawn because something has changed the size of the Viewport, it can call @Terminal.Gui.View.SetNeedsLayout.

Clipping

Clipping enables better performance by ensuring on regions of the terminal that need to be drawn actually get drawn by the @Terminal.Gui.ConsoleDriver. Terminal.Gui supports non-rectangular clip regions with @Terminal.Gui.Region. @Terminal.Gui.ConsoleDriver.Clip is the application managed clip region and is managed by @Terminal.Gui.Application. Developers cannot change this directly, but can use @Terminal.Gui.Application.ClipToScreen, @Terminal.Gui.Application.SetClip(Region), @Terminal.Gui.View.ClipToFrame, and @Terminal.Gui.ClipToViewPort.

---

I am now working through what it would mean to have Border be treated like Margin above: Be excluded from the Clip of a View and rendered in a 2nd pass. This should make the whole LineCanvas thing work much more cleanly.

[tig]

This is now ready for real review.

Still some to do, but mostly unit tests.

[tig]

Huge progress on auto-join borders and lines. I've been upgrading AllViewsTester as a hard-core test case...

Things that are not quite right. I plan on fixing these in later PRs.

• [ ] Titles get messed up - This will be fixed once Border starts actually using subviews for the border lines and title.
• [ ] I have a hack in place right now that causes all views in a view heirarchy that have SuperViewRendersLineCanvas set to redraw anytime any of them changes. This negates the perf work I've done when SuperViewRendersLineCanvas is enabled.

[tig]

@tznind, @BDisp, @dodexahedron - This is as ready as it's gonna get. Please read the first post to see all that I've done.

I'd like to get this merged tomorrow.