Another UI performance drop

[MarcelHB]

Bug description

Feels somewhat similar to #2087. I'm here and there is nothing moving:

Yet, the more text to the right, the bigger the FPS are going down. Also: scrollbar elements are missing despite being useful here. Not sure if related to this problem for now. Anyway, plenty of font rendering going on:

GemRB version (check as many as you know apply)

• master as of this issue
• 0.9.2
• 0.9.1
• 0.9.0

Video Driver (check as many as you know apply)

• SDL1.2
• SDL2 built with OPENGL_BACKEND enabled
• SDL2 without OPENGL_BACKEND enabled

[lynxlynxlynx]

That's interesting, the scrollbar actually works, we just don't display it. Maybe something goes wrong when we recreate it, since it's connected to the textarea.

I can imagine it being slow on load, since it has to layout to such short columns, but the actual rendering shouldn't be impacted by the invisible text — that sounds fishy.

[bradallred]

Maybe something goes wrong when we recreate it, since it's connected to the textarea.

we dont relocate scrollbars. We ignore the ones in the CHU if they are assigned to a TextArea and the ScrollView creates its own. They are supposed to get hidden if there is no scrollable content.

The trace is impossible to decipher, but I don't think we optimize away printing text. Font::Print will optimize away the sprite blitt, but I don't think anything stops the View from running the Draw code unless the entire View is clipped:

gemrb/gemrb/core/GUI/View.cpp

Line 252 in a8b88ee

if (intersect.size.IsInvalid()) return; // outside the window/screen

[bradallred]

yeah, see this TODO:

gemrb/gemrb/core/GUI/TextSystem/TextContainer.cpp

Line 370 in a8b88ee

// TODO: pass the clip rect so we can skip non-intersecting regions

[MarcelHB]

The trace is impossible to decipher, but I don't think we optimize away printing text. Font::Print will optimize away the sprite blitt, but I don't think anything stops the View from running the Draw code unless the entire View is clipped:

The trace just shows that the CPU is spending 95% of the drawing time of a frame in Font::Print.

Shouldn't we just not be here at all? There is no content update here, so my guess is that we do not even go into DrawSelf (of a text container), but we do because maybe some dirty flag toggling is still too sensitive? Maybe like this FIXME in View.cpp. Just did not have the time to look deeper into this.

[bradallred]

The trace just shows that the CPU is spending 95% of the drawing time of a frame in Font::Print.

Yes, but that's all it shows. I shows nothing about how you end up there. Lots of stuff prints. I don't doubt that it's the TextContainer, but it's always helpful to be sure.

maybe some dirty flag toggling is still too sensitive?

You're using a tracing tool. Have it record calls to View::MarkDirty on the TextContainer.

I suspect the ScrollBar isn't really hidden, so maybe we have a separate bug there. I think in some cases we maybe we do use the CHU scrollbar because the background is baked in (possibly all of IWD2?). So we need to figure out what's going on there. You can use the VIEWS debug flag to get a visual of the hierarchy which could be useful.

The FIXME I mentioned in #2087 should still only impact frames that are changing, it's just an issue of drawing more than actually changed.

Sounds more like another invisible view that actually does have an area, but that should still show up with the debug flag enabled.

[MarcelHB]

Yep, it's a scroll bar with the invisible flag set but a non-zero area.

Will do more research later.

[MarcelHB]

Different example but I guess the same situation:

If I make scrollbar in the red area visible via script, the framerate is good. Otherwise, there is a dirty subview because of the invisibility and non-zero area. As far as I can see, some Python statement SetVisible(False) on a scrollbar will only touch the flags but not change the size to some 0 (as in ToggleScrollbar) but that's not the case in here, so there is probably something else.

[MarcelHB]

So yeah, that's the reason apparently:

gemrb/gemrb/plugins/CHUImporter/CHUImporter.cpp

Lines 381 to 384 in 1220c11

	// NOTE: we dont delete this, because there are at least a few instances
	// where the CHU has this assigned to a text area even tho there isnt one! (BG1 GUISTORE:RUMORS, PST ContainerWindow)
	// set them invisible instead, we will unhide them in the scripts that need them
	sb->SetVisible(false);

What to do? I'd say that that SetVisible on a scrollbar should be overloaded or something to match the criteria introduced by #2087, like putting the dimension to 0 as well.

[bradallred]

Right, and that probably makes sense with IWD2 and PST because we cant just assign our own scrollbars due to the CHU background and their placement relative to the TextArea. We probably ought to have a flag to track when we are managing the scrollbar vs when it's external so we don't toggle the visibility in those cases.

But it also seems like there is a bug with the visibility toggling, because it looks like the ScrollBar should be visible in your example, no?

I'd say that that SetVisible on a scrollbar should be overloaded

I disagree; I don't think that is something that should be overloadable. If something did want to invoke such behavior we have View::FlagsChanged already. But changing the dimensions shouldn't be necessary, we do it for the other case because the ContentView is also resized to take advantage of the space when the content is short.

That said, perhaps the IsVisible check in DirtySuperViewRegions could be removed if View just called superView->MarkDirty() when visibility changes.

[bradallred]

But it also seems like there is a bug with the visibility toggling, because it looks like the ScrollBar should be visible in your example, no?

I guess that scrollbar is for the list on the left? I don't understand how that would get hidden since it doesn't seem attached to a TextArea. Something in the GUIScript? Seems like it ought to be disabled, not invisible.

[MarcelHB]

But it also seems like there is a bug with the visibility toggling, because it looks like the ScrollBar should be visible in your example, no?

No, there is nothing to scroll down. The original displays it though, not even looking disabled or anything like it, but it's useless in this case.

I guess that scrollbar is for the list on the left? I don't understand how that would get hidden since it doesn't seem attached to a TextArea. Something in the GUIScript? Seems like it ought to be disabled, not invisible.

No, it's just that in CHUImporter, and nothing apparently ever requests its visibility again.

gemrb/gemrb/plugins/CHUImporter/CHUImporter.cpp

Lines 376 to 385 in 1220c11

	TextArea* ta = GetControl<TextArea>(textareaID, win);
	if (ta) {
	ta->SetScrollbar(sb);
	} else {
	ctrl = sb;
	// NOTE: we dont delete this, because there are at least a few instances
	// where the CHU has this assigned to a text area even tho there isnt one! (BG1 GUISTORE:RUMORS, PST ContainerWindow)
	// set them invisible instead, we will unhide them in the scripts that need them
	sb->SetVisible(false);
	}

That said, perhaps the IsVisible check in DirtySuperViewRegions could be removed if View just called superView->MarkDirty() when visibility changes.

Yeah, I'll give it a try.

[lynxlynxlynx]

Re CHUImporter: can't we just delete that branch and call SetVisible on the ones that need it from the python side?

[bradallred]

Re CHUImporter: can't we just delete that branch and call SetVisible on the ones that need it from the python side?

not exactly, the ctrl = sb bit is probably relevant. You'd also then have to track down all the places it occurs. Is there an easy way to do that?

[lynxlynxlynx]

Ah, yes, didn't see that — obviously I meant just the SetVisible call. But yes, if you're asking if it's easy to find scrollbars with bad links, that's easily scriptable with iesh (just annoying, since it's still on py2). It just feels like the hiding would be a rare need.

[MarcelHB]

Looks good this way.