Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use settings ini file method #1370

Merged
merged 54 commits into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@RadWolfie
Copy link
Member

RadWolfie commented Aug 2, 2018

@Cxbx-Reloaded/developers, recommendation action is to use squash merge since 1/3 of commits are not in order. (And to prevent corruption to EmuShared after heavy modification.)

Features:

  • NEW Integrate all user's settings to settings file
  • NEW Support to ignore running Cxbx-Reloaded as administrator privilege warning message.
  • NEW Allow user to choose location for settings file to be in on first run. (Either User Profile, aka AppData, or in current working directory.)
  • UPDATED EmuShared revised v2
    • Also contain settings revision control to avoid data corruption between releases.
  • RESTORED Launch directly to kernel mode has return since mainstream settings into EmuShared is done on first launch.

Bug fixes:

  • Fixed DirectInput Controller dialog producing false "not responding" message.
  • Fixed kernel mode launch with tiny window screen.

Known issues:

  • Is not fully cross-platform yet due to Qt is not included yet.
    • I had add notes what to prepare once Qt is included.
  • Not an issue, "setup" location for settings is using MessageBox dialog.
    • Since Settings class is initialized before WndMain class is used.
  • If running GUI and perform kernel process launch directly from command prompt, then GUI will state emulator is still running until close all GUI process(es).
    • Is now using separate EmuShared memory base on GUI's process ID.
    • NOTE: Running multiple kernel processes directly at same time isn't recommended between different builds.
      • Also apply to GUI process too.

I think that's all...

For testers and contributors, please review for any regression.

RadWolfie added some commits Jul 28, 2018

bool Reserved2 = 0;
bool Reserved3 = 0;
bool Reserved4 = 0;
int Reserved99[10] = { 0 };

This comment has been minimized.

@x1nixmzeng

x1nixmzeng Aug 2, 2018

Contributor

What are all these reserved values for?

This comment has been minimized.

@RadWolfie

RadWolfie Aug 2, 2018

Author Member

1 ) Align structure
2a ) For future use if we don't need to re-update settings version every time.
2b ) Plus any new implements.

This comment has been minimized.

@RadWolfie

RadWolfie Aug 3, 2018

Author Member

Although, once proper CPU emulation is implement, all of reserves can be safely remove. Plus replace char array strings into std::string too.

@@ -183,26 +183,26 @@ INT_PTR CALLBACK DlgVideoConfigProc(HWND hWndDlg, UINT uMsg, WPARAM wParam, LPAR

lRet = SendMessage(hVideoResolution, CB_GETLBTEXT, (WPARAM)lRet, (LPARAM)szBuffer);

g_XBVideo.SetVideoResolution(szBuffer);
strncpy(g_XBVideo.szVideoResolution, szBuffer, ARRAYSIZE(g_XBVideo.szVideoResolution));

This comment has been minimized.

@x1nixmzeng

x1nixmzeng Aug 2, 2018

Contributor

This is an old mix of string allocation styles. Also ARRAYSIZE is not modern C++. With fixed buffers, prefer std::size()

This comment has been minimized.

@RadWolfie

RadWolfie Aug 2, 2018

Author Member

szVideoResolution is a char array which is not C++. EmuShared's structure cannot have any C++ in it since it is not compatible.

Using ARRAYSIZE is correct for able to support copying char array, wchar_t array, etc. Via strncpy/ wcsncpy functions or other functions might be using arrays in a variable.

For example:

char bufferStr[4];
wchar_t bufferWStr[4];
int sizeStr = ARRAYSIZE(bufferStr);
int sizeWStr = ARRAYSIZE(bufferWStr);
int sizeOfWStr = sizeof(bufferWStr);
// sizeStr equal to 4
// sizeWStr is also equal to 4
// sizeOfWStr is equal to 8.

This comment has been minimized.

@RadWolfie

RadWolfie Aug 4, 2018

Author Member

I recently discover ARRAYSIZE isn't define on other compilers. I will be using std::size for now on.

@@ -1540,7 +1542,7 @@ void CxbxInitFilePaths()
// Make sure our data folder exists :
int result = SHCreateDirectoryEx(nullptr, szFolder_CxbxReloadedData, nullptr);

This comment has been minimized.

@x1nixmzeng

x1nixmzeng Aug 2, 2018

Contributor

This should be updated to use the std::filesystem instead of the WinAPI

This comment has been minimized.

@RadWolfie

RadWolfie Aug 2, 2018

Author Member

It should, except it's not part of the changes/moving I made. There are other WinAPI will have to be take care of. Currently, I'm exhausted. 😛

This comment has been minimized.

@RadWolfie

RadWolfie Aug 4, 2018

Author Member

Done 😉

@x1nixmzeng

This comment has been minimized.

Copy link
Contributor

x1nixmzeng commented Aug 2, 2018

From a quick scan this looks faithful. There are a scary number of global data hanging around, which isn't thread safe (if that's important).

And (out of scope of this PR) the string handling is still all over the place

@RadWolfie

This comment has been minimized.

Copy link
Member Author

RadWolfie commented Aug 2, 2018

Settings class isn't meant for thread-safe. The design is to get the structure once for emulator. Or get and set the structure once by window dialog. Toggles from menu selection is different topic. Beside, window message handler is on one thread, not multiple threads.

the string handling is still all over the place

Provide two examples please? So I can understand what you're saying.

@LukeUsher

This comment has been minimized.

Copy link
Member

LukeUsher commented Aug 3, 2018

Looks good so far, although a large part of the reason for wanting a Settings class and .ini is to completely get rid of EmuShared, if we design properly, we don't need it.

The GUI can save the settings before launching the emulator process, the emulator process can then read Settings.ini during it's bootup.

Anything such as FPS updating, etc can be done via message passing to the GUI process: On Windows, this could use SendMessage(), on other platforms, this would use Unix sockets for IPC.

The same is true for settings changes: The GUI could send a message to the emulator process via the same IPC mechanism to instruct it to reload settings.

There is no need for shared memory anymore with this approach.

@RadWolfie

This comment has been minimized.

Copy link
Member Author

RadWolfie commented Aug 3, 2018

Looks good so far, although a large part of the reason for wanting a Settings class and .ini is to completely get rid of EmuShared, if we design properly, we don't need it.

Running xbox kernel thread in GUI mode when? A: Once we have proper CPU emulation. It's a major blockage for both implement Qt and integrate xbox kernel process into GUI process to start emulator.

Anything such as FPS updating, etc can be done via message passing to the GUI process: On Windows, this could use SendMessage(), on other platforms, this would use Unix sockets for IPC.

Pretty sure it require Qt for cross-platform support. Unless there is a std library for it.

The GUI could send a message to the emulator process via the same IPC mechanism to instruct it to reload settings.

It doesn't work that way when you have two GUI process running at same time which can contain any modification at any time. Plus settings are not save to hard drive until Cxbx's GUI process is close. Also doesn't support multi-xbe either. Unless you want to re-save to hard drive every time on start emulation.

EDIT: @LukeUsher, if you do choose go down your way. You will get more false positive reports from the community. Unless migrate emulation into one process instead of start kernel process separately.

The GUI can save the settings before launching the emulator process, the emulator process can then read Settings.ini during it's bootup.

Hmm, I didn't see this sentence. My response is, I'm not interesting in frequent writes to the hard drive.

RadWolfie added some commits Aug 3, 2018

Implement support to prevent tamper shared memory between GUI processes.
NOTE: Kernel process will be using same shared memory since guiProcessID will remain as 0.
Various improvement
* Use std library functions
* Use single column spaces for align support on different tab size
* Include failsafe if custom and file path string values are bad format

Apparently ARRAYSIZE isn't define on other compilers.
@RadWolfie

This comment has been minimized.

Copy link
Member Author

RadWolfie commented Aug 4, 2018

Currently, load settings from a file has all of custom value, is a string, and strings has fail safe setup.

@RadWolfie

This comment has been minimized.

Copy link
Member Author

RadWolfie commented Aug 4, 2018

Current state: "Ready for final testing and possible squash merge"

Revert support for 2+ emulation process at same time.
Emulation's graphic screen has corruption between both windows. Not even a wrapper to D3D9 will fix it.

@LukeUsher LukeUsher merged commit 07a05f0 into Cxbx-Reloaded:develop Aug 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@RadWolfie RadWolfie deleted the RadWolfie:use-settings-ini-file-format branch Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.