Fix against severe FPS drops by scrollbar

[MarcelHB]

Description

I noticed that opening some text sections, like a mildly filled journal or dialogs, drops FPS by up to 75%.

That is because 382ed93 checks whether invisibility is set to get a background redraw of something after turning invisible. Problem: scrollbars may be invisible by design, so that neighbouring content view is marked dirty and thus it draws fonts over and over again, even if nothing actually requires redrawing.

I made ScrollBar exempt from counting for this invisibility check, not sure if the name suits well but I still can't come up with any better one.

Checklist

• Commit messages are descriptive and explain the rationale for changes
• I used the same coding style as the surrounding code
• I have tested the proposed changes
• I extended the documentation, if necessary
• The proposed change builds also on our build bots (check after submission)

[lynxlynxlynx]

LGTM, @bradallred ?

[bradallred]

Feels like a hack; tho I must admit it's not clear to me what exactly is happening. I suspect this might cause issues with some places with nested views. At a minimum, the name is indeed confusing.

IIRC, ScrollView contains both a ContentView and a neighboring ScrollBar sized to fill the ScrollView. They don't nest/overlap each other. So the problem is basically the FIXME in InvalidateDirtySubviewRegions (lack of partial redraws)? Which, ofc there is no easy fix for. I can see why we would reach for this simple solution.

Seems that the visibility check is there to handle when views change visibility only, so just a single frame of redrawing is needed. Maybe some way of tracking that then? Maybe dirty should be a bitfield so we can distinguish between the view needing to redraw and the superview needing to redraw the area the view occupies. Certainly more work, but it could also potentially replace IsAnimated and IsOpaque down the road. Setting a bit during a relevant state change is much less complex and timely than calling various methods every frame.

We can punt on it for now and commit this if that is too much to ask or otherwise infeasible.

[MarcelHB]

Feels like a hack; tho I must admit it's not clear to me what exactly is happening. I suspect this might cause issues with some places with nested views. At a minimum, the name is indeed confusing.

Yeah. Maybe you remember the case you were fixing in this commit, i. e. the example?

IIRC, ScrollView contains both a ContentView and a neighboring ScrollBar sized to fill the ScrollView. They don't nest/overlap each other. So the problem is basically the FIXME in InvalidateDirtySubviewRegions (lack of partial redraws)? Which, ofc there is no easy fix for. I can see why we would reach for this simple solution.

Not sure if this is actually helpful if we had that (I just did not do the math for the regions). Structure is:

ScrollView

• ScrollBar
• ContentView
  • TextContainer

So ScrollBar is invisible due to short text, ScrollView notices that via InvalidateDirtySubviewRegions and the scroll bar's non-empty DirtySuperViewRegions because of its own invisibility, so ScrollView is now dirty and thus it calls InvalidateSubviews that will make ContentView dirty and redraw itself and the children views.

Seems that the visibility check is there to handle when views change visibility only, so just a single frame of redrawing is needed. Maybe some way of tracking that then? Maybe dirty should be a bitfield so we can distinguish between the view needing to redraw and the superview needing to redraw the area the view occupies. Certainly more work, but it could also potentially replace IsAnimated and IsOpaque down the road. Setting a bit during a relevant state change is much less complex and timely than calling various methods every frame.

Sure, that is probably the clean way to do but I expect this to be some more work with regressions to look out for.

[bradallred]

Yeah. Maybe you remember the case you were fixing in this commit, i. e. the example?

I don't. There may be some neighboring commits with clues.

So ScrollBar is invisible due to short text, ScrollView notices that via InvalidateDirtySubviewRegions and the scroll bar's non-empty DirtySuperViewRegions because of its own invisibility, so ScrollView is now dirty and thus it calls InvalidateSubviews that will make ContentView dirty and redraw itself and the children views.

Right, and we do that because there is no mechanism in place for partial redrawing

All that aside, I think there is possibly an easier fix. IIRC, ScrollView hidden scrollbars are also resized so the ContentView can occupy that space. Seems like we should just add code to check that the View area is not 0 before returning it in DirtySuperViewRegions.

[MarcelHB]

You mean like this? IsZero() won't work since it checks for both w and h being 0 whereas IsInvalid() is happy with one of them being 0 as it is the case for the ScrollBar.

diff --git a/gemrb/core/GUI/View.cpp b/gemrb/core/GUI/View.cpp
index 731ad3bfe..76227c1c8 100644
--- a/gemrb/core/GUI/View.cpp
+++ b/gemrb/core/GUI/View.cpp
@@ -177,7 +177,7 @@ Regions View::DirtySuperViewRegions() const
                return {};
        }

-       if (NeedsDraw() || (!IsVisible() && !HasIntrinsicInvisibility())) {
+       if ((NeedsDraw() || !IsVisible()) && !frame.size.IsInvalid()) {
                return { frame };
        }

Works.

[bradallred]

yes, roughly, except I was thinking is would be a condition for the case returning an empty set (like IsOpaque), because if a View has 0 area then its subviews shouldn't matter either, so no need to even check those.

If it works, lets do that.

[MarcelHB]

Alright, here we go.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.8% Duplication on New Code

See analysis details on SonarCloud

[bradallred]

Thanks, great work diagnosing too.