Settings window

Open-source port of PoP that runs natively on Windows, Linux, etc.

Moderator: English Moderator Team

User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 2979
Joined: April 9th, 2009, 10:58 pm
Contact:

Settings window

Postby Norbert » July 1st, 2017, 7:20 pm

I've added code for a Settings window to this fork:
https://github.com/EndeavourAccuracy/SDLPoP

It's a work-in-progress and still partially non-functional. Basically, you mouse click the main window and a Settings window shows up. Changes in the "live" column are meant to update the game on-the-fly (in real-time), and changes in the "INI" column are meant to be saved with the "Save" button. I'm hoping Falcury will help me out with some things, I'll mention below. For some other things David's knowledge of the code base and SDL may be helpful.

Some issues that maybe David could look into:
- For some reason Alt+F4 (or clicking the close cross) no longer works.
- Clicking the main window again should (and does) hide the Settings window, but making the Settings window visible again - for some reason - requires two clicks. I don't understand why.

Falcury, where it says "TODO: Save all settings" could you make it save all the INI settings to sPathFile. (sPathFile is already either SDLPoP.ini or mod.ini.) Of course there's no hurry, we're all busy with our jobs and such. I'm asking you because you've worked with/created the 'INI system' and are familiar with related variable names.
[Edit: I mean literally all INI settings, not just what is currently available on the Settings window.]

Also, some variables, such as "enable_controller_rumble", should be split in two. One variable should be used to store the value that was read from the INI file, and one variable should copy that value to be used by the actual game. This will allow the Settings window to both change on-the-fly and keep track of INI settings. For several variables this is already happening, e.g. "start_minutes_left" (INI) and "rem_min" (live).

Keeping all the above in mind, the options currently available on the Settings window already provide some functionality to make real-time changes. It's just that for the game pad rumble, changing the INI setting impacts the live settings and vice versa, minutes left is only visible when pressing Space in-game, and hit points left is limited by the max HP and the game only updates the indicators after taking damage.

I think I've done a proper job making the code flexible enough to easily add additional options to modify. Plus the Settings window already has 7 tabs, that, overall, should provide a fair amount of space for these options.
Attachments
settings.png
Settings window
Last edited by Norbert on July 1st, 2017, 7:46 pm, edited 1 time in total.

User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 2979
Joined: April 9th, 2009, 10:58 pm
Contact:

Re: Settings window

Postby Norbert » July 1st, 2017, 7:31 pm

I'm actually already having fun with this and I think this whole thing could be both useful and entertaining.
When the game starts, wait for the intro to reach the demo fight sequence.
Then open the settings and you can keep the "live" hit points left on 4 to make sure the prince always wins. :)
I'm totally looking forward to being able to turn on and off David's new lighting effects whenever I please.
I'll make sure entering numbers will also be possible by just typing them, not just by mouse clicking arrows.

David
The Prince of Persia
The Prince of Persia
Posts: 1362
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: Settings window

Postby David » July 8th, 2017, 10:35 am

Norbert wrote:I've added code for a Settings window to this fork:
https://github.com/EndeavourAccuracy/SDLPoP

The settings-window commits are on master, maybe you should move them to a new branch?
(I can help you if you don't know how to do that.)


Norbert wrote:- For some reason Alt+F4 (or clicking the close cross) no longer works.

The SDL docs say:
http://wiki.libsdl.org/SDL_EventType#SDL_QUIT wrote:An SDL_QUIT event is generated when the user clicks on the close button of the last existing window. This happens in addition to the SDL_WINDOWEVENT/SDL_WINDOWEVENT_CLOSE event, so the application can check whichever is appropriate, or both, or neither.

There are two windows now, so neither is the last, so there is no SQL_QUIT sent.
You should handle the SDL_WINDOWEVENT with the SDL_WINDOWEVENT_CLOSE subtype instead.

Something like this:

Code: Select all

               case SDL_WINDOWEVENT_CLOSE:
                  if (event.window.windowID == SDL_GetWindowID(window_)) { // main window
                     quit(0);
                  }
                  if (event.window.windowID == SDL_GetWindowID(window2_)) { // settings window
                     SDL_HideWindow(window2_);
                  }
                  break;

Maybe windowID should be checked first (in an outermost "if"), and the settings window could have its own separate event handler function.
Currently I can control the prince even it the settings window is active. :)
But not all events have a windowID...

Btw, window_ and window2_ could use a better name...


Norbert wrote:- Clicking the main window again should (and does) hide the Settings window, but making the Settings window visible again - for some reason - requires two clicks. I don't understand why.

I get something different:
When the settings window is focused, the first click on the main window just focuses the main window.
(It might cover the settings window, but it's not closed.)
The second click hides the settings window, and the third click reopens it.

Are you sure this isn't what happens for you?


Norbert wrote:and changes in the "INI" column are meant to be saved with the "Save" button.

What are we going to do with comments?
Maybe just hardcode them, like DOSBox does? (In DOSBox you can save a the config file with config -writeconf <filename>.)

And what about the commented-out options in the [CustomGameplay] section?
Comment out each option if and only if it has the default value?
Or just never comment out any option?


As for your ShowText() and related functions, err...
Since the font is the same, I think you could just use show_text_with_color().
It writes to current_target_surface, so you need to save, change, and restore it, though.

... Except that the settings window has no associated Surface, only a Renderer. :?
I guess I should rewrite my code at some time to use Textures and Renderers instead of Surfaces?...

User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 2979
Joined: April 9th, 2009, 10:58 pm
Contact:

Re: Settings window

Postby Norbert » July 8th, 2017, 12:11 pm

> Something like this: [...]

Doesn't seem to work here.

> Are you sure this isn't what happens for you?

Yes, what you describe does not match what I get.

> Maybe just hardcode them, [...]

Sounds good.

> Or just never comment out any option?

Sounds like a good solution.

> I guess I should rewrite my code at some time to use Textures and Renderers instead of Surfaces?...

I think so.
Quoting from the Migration Guide: "SDL2 still has SDL_Surface, but what you want, if possible, is the new SDL_Texture."

Falcury
Wise Scribe
Wise Scribe
Posts: 294
Joined: June 25th, 2009, 10:01 pm

Re: Settings window

Postby Falcury » July 8th, 2017, 12:32 pm

This is quite inspiring! :)
I have to think about this a bit more...

Norbert wrote:Falcury, where it says "TODO: Save all settings" could you make it save all the INI settings to sPathFile. (sPathFile is already either SDLPoP.ini or mod.ini.)


David wrote:What are we going to do with comments?
Maybe just hardcode them, like DOSBox does? (In DOSBox you can save a the config file with config -writeconf <filename>.)

And what about the commented-out options in the [CustomGameplay] section?
Comment out each option if and only if it has the default value?
Or just never comment out any option?


Hm, I think we could try to just let the raw text be preserved... The INI file including all the comments would be initially read into memory as a kind of 'template'. Then, for saving the changed configuration, the option values themselves would be inserted to replace the original option values (or, if one some option is missing, appended at the end of the corresponding section).

Norbert wrote:Also, some variables, such as "enable_controller_rumble", should be split in two. One variable should be used to store the value that was read from the INI file, and one variable should copy that value to be used by the actual game. This will allow the Settings window to both change on-the-fly and keep track of INI settings. For several variables this is already happening, e.g. "start_minutes_left" (INI) and "rem_min" (live).

So then maybe we should again create some sort of data structure that keeps together all of the settings (like the old options_type struct that was removed some time ago)?

David wrote:Btw, window_ and window2_ could use a better name...

Maybe main_window and settings_window?
And we don't really need the trailing underscores, I suppose? They probably signify that it's a global variable, but most global variables in the codebase don't have them.

David
The Prince of Persia
The Prince of Persia
Posts: 1362
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: Settings window

Postby David » July 9th, 2017, 11:33 am

Falcury wrote:
David wrote:Btw, window_ and window2_ could use a better name...

Maybe main_window and settings_window?

Something like that.

Falcury wrote:And we don't really need the trailing underscores, I suppose? They probably signify that it's a global variable, but most global variables in the codebase don't have them.

That one has an interesting tale...
In the original (DOS) prince.exe, onscreen_surface is not a pointer, but a statically allocated global struct.
But SDL always allocates surfaces dynamically, and they must be accessed through pointers.
So, to preserve what the disassembly did, I wrote something like this:

Code: Select all

extern SDL_Surface* onscreen_surface_;
#define onscreen_surface (*onscreen_surface_)
...
onscreen_surface.field = something;
current_target_surface = &onscreen_surface;

That was quite pointless, and I removed this silliness before the first release.
However, the name remained.

As you said, we don't need the trailing underscores.
I guess it's time to delete the far/near attributes as well, together with the __pascal calling convention.


Return to “SDLPoP”

Who is online

Users browsing this forum: No registered users and 1 guest