Settings window

Open-source port of PoP that runs natively on Windows, Linux, etc.
Locked
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Settings window

Post by Norbert »

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 window
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: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

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: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: Settings window

Post by David »

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: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

> 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
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: Settings window

Post by Falcury »

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: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: Settings window

Post by David »

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.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

Norbert wrote:I'm totally looking forward to being able to turn on and off David's new lighting effects whenever I please.
Took me a while, but I made that commit.

Maybe the number of torches in the room could impact the darkness of the overlay.
Would make it feel like you're really there, if it's very dark without torches...
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

Norbert wrote: July 1st, 2017, 7:20 pmFalcury, 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.]
Falcury, let's take, for example, "enable_mixer".
The "live" column should make this toggleable, plus I need to remember the SDLPoP.ini value in case the user chooses to Save all values from the INI columns.
Now, data.h has enable_mixer, and it appears to only be used in options.c as follows:

Code: Select all

process_boolean("enable_mixer", &enable_mixer);
where process_boolean() is:

Code: Select all

#define process_boolean(option_name, target)
if (ini_process_boolean(name, value, option_name, target)) return 1;
There, name and value come from the global_ini_callback() parameters, I guess.

Where/how does all this influence whether a mixer is used and music played...
Actually, I don't think that option is doing anything, because setting it to false doesn't disable music.

Is this an exception or have more options not been implemented yet?
Falcury
Calif
Calif
Posts: 565
Joined: June 25th, 2009, 10:01 pm

Re: Settings window

Post by Falcury »

Norbert wrote: November 1st, 2017, 11:16 pm Where/how does all this influence whether a mixer is used and music played...
Actually, I don't think that option is doing anything, because setting it to false doesn't disable music.

Is this an exception or have more options not been implemented yet?
I think this is an exception. Yeah, it looks like enable_mixer is unimplemented.
(Of course, if we make all of the options toggleable in-game, we'll undoubtedly run into some more bugs. But I expect they should be mostly straightforward to fix.)

In order to disable music at startup, one easy fix is to make this line in load_sound() in seg009.c conditional on enable_mixer:

Code: Select all

if (enable_mixer && !digi_unavailable && result == NULL && index >= 0 && index < max_sound_id) {
Although that doesn't yet allow for toggling the music while in-game.
I could look into it.

Maybe the option should be renamed to enable_music as well?
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

Norbert wrote: July 1st, 2017, 7:20 pm- 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've looked into this again, and by now I'm fairly certain this is just another SDL bug. May be Linux only, but I found this bug report which was updated Nov. 2017, and this patch from July 2017. It's very simple, really; I've tested SDL_HideWindow() and SDL_ShowWindow() in many ways and one or both simply aren't doing what they should be doing according to the documentation. At least not with 2.0.4. So, even if it's not related to the aforementioned bug report and patch, it's still an SDL issue.
David wrote: July 8th, 2017, 10:35 am
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;
Right, thanks.
Inside the case SDL_WINDOWEVENT I can check if event.window.event is case SDL_WINDOWEVENT_CLOSE.
That works.
(And Ctrl+c on the console still sends SDL_QUIT.)
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

David wrote: July 8th, 2017, 10:35 amBut not all events have a windowID...
That's true, for example with SDL_CONTROLLERBUTTONUP.
However, it should be possible to use the window.windowID that has keyboard focus.
And keyboard focus can be tracked and stored via SDL_WINDOWEVENT_FOCUS_GAINED, a case of SDL_WINDOWEVENT.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: Settings window

Post by Norbert »

Feel free to close this thread.
Its main legacy is that it co-inspired Falcury. :)
We'll 'obviously' be using the, by now clearly superior, pause menu instead.
Locked