[Yarn]

I updated the patch so that it applies cleanly to the current trunk. The build has also been updated. Help overlays are still limited to two resolutions; I'm working on fixing this now, although I'm having some difficulty with getting the correct lines to be drawn. If I can't sort this out, then I'll just post what I've got and let you guys find out what's wrong.

[Yarn]

I have updated the patch and build again. Help.tbl has now been upgraded to support more than two resolutions; see this post for details. The example mod has also been modified to utilize this feature. (Creating an overlay without a graphical editor is a real PITA, so the mod's overlays don't have a label for every button.)

If no bugs are found, then this patch is ready for review.

[niffiwan]

I know it's not a brilliant solution, but I found the "help_reload" function in the debug console made it a lot easier to modify the help overlays.  i.e. instead of

1) start FSO, choose pilot, press F1, exit FSO, modify table, restart

you can do:

2) press F1, modify table, start debug console, type "help_reload" (or up-arrow), press F1

Anyway, if Mjn can test the feature, I can review the code.

[Yarn]

I made a slight tweak to the patch: Instead of +Help Overlay Resolution Index defaulting to 0 for the first main-hall resolution and 1 for the others, it's now the same as the main hall's own resolution index (0 for the first, 1 for the second, 2 for the third, etc.). (If the overlay is not available in the requested resolution, then no overlay will appear.)

Now I think the patch is really ready for review.

[mjn.mixael]

I'm not all too familiar with the help stuff.. but I did some basic tests and everything seems to work quite nicely! I've no complaints.

[Zacam]

I'm also liking the change very much and haven't noticed anything broken, though I still need remember to access the options screen while in mission. Cause I keep forgetting.

[AdmiralRalwood]

I have a slight problem with the current patch:



Other than that, it all looks good to me.

[Yarn]

I updated the patch and builds again to fix the problem that AdmiralRalwood described above.

[m!m]

The patch looks good to me. I guess this can be put into trunk.

[niffiwan]

I've finally reviewed the patch (sorry about the tardiness ).

Overall it looks good, both test mods seem to run fine on Linux when testing with 4:3, 16:9 & 5:4 resolutions (all tested in a window).

On the code, I've got a few comments (uh oh).

1)

Code: (code/gamehelp/contexthelp.cpp) [Select]

+ //mprintf(("Found text %d on overlay %d - location (%d,%d) @ 640x480 :: location (%d,%d) @ 1024x768\n", currcount, overlay_id, help_overlaylist[overlay_id].textlist[GR_640][currcount].x_coord, help_overlaylist[overlay_id].textlist[GR_640][currcount].y_coord, help_overlaylist[overlay_id].textlist[GR_1024][currcount].x_coord, help_overlaylist[overlay_id].textlist[GR_1024][currcount].x_coord));
...
+ //mprintf(("Found rbracket %d on overlay %d - location (%d,%d) @ 640x480 :: location (%d,%d) @ 1024x768\n", currcount, overlay_id, help_overlaylist[overlay_id].rbracketlist[GR_640][currcount].x_coord, help_overlaylist[overlay_id].rbracketlist[GR_640][currcount].y_coord, help_overlaylist[overlay_id].rbracketlist[GR_1024][currcount].x_coord, help_overlaylist[overlay_id].rbracketlist[GR_1024][currcount].y_coord));
...
+ //mprintf(("Found lbracket %d on overlay %d - location (%d,%d) @ 640x480 :: location (%d,%d) @ 1024x768\n", currcount, overlay_id, help_overlaylist[overlay_id].lbracketlist[GR_640][currcount].x_coord, help_overlaylist[overlay_id].lbracketlist[GR_640][currcount].y_coord, help_overlaylist[overlay_id].lbracketlist[GR_1024][currcount].x_coord, help_overlaylist[overlay_id].lbracketlist[GR_1024][currcount].y_coord));

Maybe remove the commented out mprintf's, or convert them to nprintf's? (maybe Parse or a new category called Help?)
(I know these are existing comments, but a cleanup would be good).

2)
In the following places the code should probably be removed rather than commented out.
code/menuui/mainhallmenu.cpp (line 354->363, and 1762->1779, approx 1970)

3)
When setting a custom font for a mainhall, should the font be saved first (with gr_get_current_fontnum()) and restored to the previous value instead of always setting it back to FONT1?
i.e. this is used several times
gr_set_font(Main_hall->font);
(code)
gr_set_font(FONT1);
(Or it is known that the font is always FONT1 prior to this point?)

4)
These statements don't compile using GCC:
(code/gamehelp/contexthelp.cpp)

Code: [Select]

+ SCP_vector<SCP_vector<help_pline>> plinelist;
+ SCP_vector<SCP_vector<help_text>> textlist;
+ SCP_vector<SCP_vector<help_left_bracket>> lbracketlist;
+ SCP_vector<SCP_vector<help_right_bracket>> rbracketlist;

Just add a space between the >> and it's fine; i.e.

Code: [Select]

+ SCP_vector<SCP_vector<help_pline> > plinelist;
+ SCP_vector<SCP_vector<help_text> > textlist;
+ SCP_vector<SCP_vector<help_left_bracket> > lbracketlist;
+ SCP_vector<SCP_vector<help_right_bracket> > rbracketlist;

Otherwise it looks good

[Yarn]

On the code, I've got a few comments (uh oh).

...

Regarding your first comment, it's probably a good idea to delete those lines, considering that they assume that only two resolutions exist (which isn't necessarily true with this patch). I'll make this change and the other changes that you suggested in the next version of the patch, which should be ready within 24 hours. EDIT: It's updated now!

[niffiwan]

Thanks!  The new patch looks good, I'll commit it tonight (unless someone else gets there 1st)

[Zacam]

I'd like (for the sake of OCD) to suggest that lines 208 & 226 see a slight change for the actual commit:

Code: [Select]

+ if (overlay_id >= 0 && overlay_id < num_help_overlays && resolution_index >= 0 && resolution_index < help_overlaylist[overlay_id].num_resolutions) {
...
+ if ( overlay_id >= 0 && (Help_overlay_flags & (1<<overlay_id)) && resolution_index >= 0 && resolution_index < help_overlaylist[overlay_id].num_resolutions ) {

into:

Code: [Select]

+ if ( (overlay_id >= 0) && (overlay_id < num_help_overlays) && (resolution_index >= 0) && (resolution_index < help_overlaylist[overlay_id].num_resolutions) ) {
...
+ if ( (overlay_id >= 0) && (Help_overlay_flags & (1<<overlay_id)) && (resolution_index >= 0) && (resolution_index < help_overlaylist[overlay_id].num_resolutions) ) {

This will help in readability at a minimum even if it affects nothing else.

[niffiwan]

Committed in r10844, including Zacam's suggestion, a minor line-breaks addition from me on the same lines and all trailing whitespace removed.  Thank you Yarn

[zookeeper]

Unfortunately, the patch (r10844) seems to break cockpit displays. I suspect the changes to hud.cpp, but haven't confirmed if it's that or something else; I do see some traces of the HUD elements on the cockpit (in the wrong places), which is why I think it has something to do with stretching/scaling going wrong. I don't have a testcase to provide though.

[mjn.mixael]

I'm guessing you didn't create a mantis ticket with a test case.

[zookeeper]

Aaand here's even a ticket with a testcase.

[Nyctaeus]

Is this changed? I play on some old Nightly of 3.7.1 to have my interface unstreched. Sorry guys for being lazy asshat, but four pages of text caused tl;dr syndrome

[AdmiralRalwood]

Is this changed? I play on some old Nightly of 3.7.1 to have my interface unstreched. Sorry guys for being lazy asshat, but four pages of text caused tl;dr syndrome

This patch has been in trunk since r10844. I'm not sure what you mean by "some old Nightly of 3.7.1 to have my interface unstretched", unless... are you saying you've been using an old version of FSO to avoid pillarboxing? That is what the -stretch_menu command-line option is for (and would be the opposite of "unstretched").

[Nyctaeus]

To be precise I'm using 10451 to avoid black bars on either side of the game window in newer builds. I was thinking that something is wrong with my PC and I'm looking for some solution. Custom flag worked, thanks for the help.