Helion-Engine / Helion

A modern fast paced Doom FPS engine
GNU General Public License v3.0
86 stars 9 forks source link

Add help-text footer to Options menu #582

Closed lemming104 closed 2 months ago

lemming104 commented 2 months ago

On all options sections except key bindings, add a footer that explains the currently-selected option, based on the description of the underlying config item.

Note that this doesn't "reserve" an area at the bottom of the screen, so it is possible for a long Options section to run over the footer. However, this is currently only a risk for the key bindings section, because none of the others are anywhere near long enough for this to be a problem, at typical modern display resolutions.

nstlaurent commented 2 months ago

This is good, I was actually thinking about looking into this soon. I would suggest adding something very similar to the error/warning messages box that can be found here:

https://github.com/Helion-Engine/Helion/blob/cfa5450f9d14e01c22fd041b94ca2e18b03e0f7b/Core/Layer/Options/OptionsLayer.cs#L425

It ensures that it's rendered on top and gives it a nice background box if there is text behind it: image

lemming104 commented 2 months ago

OK, I'll play around with making it render that way.

lemming104 commented 2 months ago

Done; I think it does what you want now.

nstlaurent commented 2 months ago

This ended up being more complicated than I thought. I run hud.scale set to 3. It currently renders behind text so it needs to be almost last in the rendering but before the error/warning message. I think the scrolling should add some padding to the bottom of the page in the calculations so the text is able to scroll above the message.

I also realized that using RenderableString directly does mostly what we want. But it's just not capable of breaking on words right now. (You would have to to pass the ArchiveCollection to the OptionsLayer for this code to work.)

` private void WriteMultilineFooter(string inputText, string font, int fontSize, IHudRenderContext hud) { if (string.IsNullOrEmpty(inputText)) return;

   int padding = m_config.Hud.GetScaled(8);
   var fontObject = m_archiveCollection.GetFont(font);
   var str = new RenderableString(m_archiveCollection.DataCache,
       inputText,
       fontObject, fontSize, drawColor: Color.White, maxWidth: hud.Width - padding * 2);

   hud.FillBox(
       new(new Vec2I(0, hud.Dimension.Height - str.DrawArea.Height), new Vec2I(hud.Dimension.Width, hud.Dimension.Height)),
       Color.Black,
       alpha: 0.7f);

   hud.Text(str, (0, 0), both: Align.BottomMiddle);
   return;

} `

I can work on making the RenderableString break on words as it probably should. The code is here if you wanted to take a look: https://github.com/Helion-Engine/Helion/blob/dev/Core/Render/OpenGL/Texture/Fonts/RenderableString.cs#L106

Just keep in mind this class gets used in the rendering loop during gameplay so it does zero allocation, so we can't use string splits or anything. Would likely have to backtrack.

lemming104 commented 2 months ago

I'll see if I can mess around with the render order a bit and get it not to interfere with the scroll area. I didn't realize it was possible to come up with a combination of settings that would make the other Options sections need a scroll bar.

For now, I'll try to stay out of the RenderableString stuff; I do a lot of C# programming, but I'm usually writing stuff like web servers where we're a little more willing to accept garbage collection here and there. :)

lemming104 commented 2 months ago

See if you like this any better. There's still some hackery trying to work around the ever-changing dimensions of the footer (which I'm still resizing based on how much room I need for the footer text), so the scroll bar display isn't perfect, but with the default "windowed" size (1024 x 768?) and HUD size 2, which causes several of the options pages to scroll, the menus now seem pretty usable to me.

lemming104 commented 2 months ago

Further simplified it; scroll bar position is now just based on selected row index, only on options sections that can scroll. Feels more natural to me than the current mainline behavior.

nstlaurent commented 2 months ago

Are you using the keyboard or the mouse? I generally use the mouse and that is what it is more geared for. This change feels really awkward. I think previously a big part of the issue is there is code that calculates the render size and scroll bar size using the page, but one probably isn't account for this new padding at the bottom.

lemming104 commented 2 months ago

I tried using both the keyboard and the mouse, and it seemed generally reasonable, although having the bar position update with mouse hover may be undesirable. As far as I can tell, the user can't directly interact with the scroll bar, right?

I can see that it seems like having the scroll bar move based on where the mouse is hovering seems odd, but the previous behavior isn't really very intuitive either--Windows and so on typically resize the graphic for the scroll bar based on how much of the list is in the current view. If you've got a settings page that is exactly one line longer than the screen, the current mainline behavior is that you have a tiny little scroll bar "beam" that will jump from the very top to the very bottom when the user goes to the last visible item and then advances one more line.

I can certainly revert that last change, but it still leaves an annoying problem. Basically, we need to bump the scroll to afford some room for the footer, but the size of the footer isn't constant, and depends upon which item is currently selected. It's easy to end up with a behavior where the scroll bar sort of "jitters" up and down based on how much padding we've left for the footer (which varies, depending on the selected item), and that feels weird too. If I don't accommodate for the amount of padding added by the footer, then that results in the scroll bar hitting the end a bit early, which isn't right either. Maybe calculate the scroll offset twice--once for the relative position used for the scroll bar, and again for the actual position when we bump stuff for the footer?

Another option would be to just calculate the most space we could possibly need for the footer and keep it constant throughout an options group. That would prevent stuff from dancing around, but could result in wasted space in the footer area, which seems like it'd be undesirable when running at larger HUD font scales.

Yet another way would be to not display the descriptions all the time, and just display them on-demand when the user presses F1 or right-clicks or something, and put some text in the header to tell them they can do this. That avoids the root problem here, which is that I'm trying to impose GUI-like metaphors in an environment where we're really just rendering one big slab of text and scrolling the whole thing in unison. I mean, the "header" text currently scrolls right off the top of the screen, which isn't something you'd normally expect in a GUI.

What design do you prefer? Do you like the idea of having a permanent footer like this? Should it resize or stay constant size? Or should the whole thing just display when the user specifically asks what a setting does? Or something else entirely?

I would like to give the user some information about what the settings mean, because as a user, I don't actually know what some of them do. :) However, I think I may be trying to fit a round peg into a square hole with this approach.

lemming104 commented 2 months ago

Here is a different way I could achieve the same end goal (helping the user to understand the settings) while avoiding concerns related to scrolling and pagination.

https://github.com/Helion-Engine/Helion/compare/dev...lemming104:Helion:lemming104/OptionsFooter_OnDemandOnly?expand=1

In this version, I am just hijacking the existing "message box" idea to display help text on-demand when the user presses F1 or right-clicks on the menu. As with the warnings that are normally displayed in the message box when the user enters an invalid value for a setting, the box just goes away after 5s of inactivity, or when the user attempts to edit the value of a setting.

If you prefer that approach, I can cancel this pull request and open one from that branch. I could see arguments either way--it's less intrusive and messes with the UI less, but it's also less persistently "helpful".

nstlaurent commented 2 months ago

I think what you had the first time was good, I was just against the changing of how the scroll handling was changed. I will merge it in with the first method of always rendering the text without the changes to making the scroll bar move per row selection. The scroll handling could use some work but I think that should be addressed separately.

lemming104 commented 2 months ago

Fair. :) That was at least a workable state, even if the scrollbar display wasn't quite right.

If you want, I can revert back to that state, then you can just squash-merge this PR (so you don't get a bunch of try-then-revert garbage in your own history).

nstlaurent commented 2 months ago

Yeah, that sounds perfect. Thanks!

lemming104 commented 2 months ago

OK just pushed the line-by-line revert 59e0b7d; it just appears in an odd place in this conversation history because I actually did that change locally yesterday and just didn't push it yet.

As you know, everything basically works, the scroll bar just hits the end a little early. It may take some messing around to find what feels most "right" for how to fudge the scroll bar position, since the height of the footer needs to be compensated for, but it's also really quite variable.

Probably best to just squash-merge this to keep the commit history of dev clean, then one of us can clean up the scroll bar position math in another change.

nstlaurent commented 2 months ago

Thanks!