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

Standardize File Handling with std::filesystem and Expand Cross-Platform Utilities #260

Merged
merged 9 commits into from
Nov 19, 2024

Conversation

stevewgr
Copy link
Member

@stevewgr stevewgr commented Nov 13, 2024

Description

This pull request represents a major improvement and modernization of the project’s file handling, path manipulation, string utilities, and logging mechanisms. Key changes include replacing legacy and platform-specific implementations with C++17 and C++20 standards, adding versatile utility functions, and enhancing debugging output for better developer experience and platform compatibility.

It also addresses variable naming inconsistencies within the codebase, particularly when migrating to std::filesystem. Recommended naming conventions for file paths and their components are as follows:

  • C:/hello/world/file.txt -> fsFile or fsFileAbs depending on the context
  • C:/hello/world -> fsDir
  • world/file.txt -> fsFile or fsFileRel depending on the context
  • file.txt -> fsFileName
  • file -> fsFileStem or fsStem depending on the context

Note: Avoid using fsFilePath, except as a function name (e.g., CN3BaseFileAccess::FilePath()). Including “Path” in variable names is redundant, as the fs::path type and fs prefix already indicate the variable represents a filesystem path.

Highlights

  1. Standardize File Handling with std::filesystem

    • Migrated file and path handling to std::filesystem, replacing numerous WinAPI and custom functions.
    • Enhanced code readability, reduced platform dependencies, and ensured consistent behavior across operating systems.
    • Cleaned up legacy code, removing deprecated components and redundant functions, simplifying maintenance.
  2. Extended Filesystem Wrapper Class fs::pathx

    • Introduced fs::pathx, a custom wrapper around std::filesystem::path, to address limitations in the standard.
    • Features automatic path normalization, extended path manipulation (case-insensitive comparisons, relative paths, etc.), and overloading for intuitive path concatenation.
    • Added utilities like mktemp_file for creating temporary files, streamlining file-handling tasks.
  3. Refactored Logging with CLogWriter

    • Updated CLogWriter to use std::fstream for portability, std::chrono for timestamps, and std::mutex for thread safety.
    • Logs are now also written to the console for immediate debugging visibility.
    • Simplified and standardized log formatting using std::format.
  4. Enhanced Utilities with Platform-Agnostic String Functions

    • Refactored N3Utils.h to include case-insensitive string utilities and cross-platform compatibility mappings.
    • Introduced concepts for string and path types, enabling cleaner and more reusable code.
  5. Improved Submodule Handling

    • Disabled recursive submodule initialization to speed up cloning.
    • Updated the ko-db submodule to include its latest release (v1.2.0), aligning it with the changes in this PR.

Benefits

These updates modernize the codebase, making it more efficient, portable, and easier to maintain. By adopting modern C++ standards and enhancing utilities, the project is better equipped for future growth and cross-platform support.

Testing and Validation

Benefits

These updates provide a more modern and efficient codebase, improving cross-platform support, developer experience, and maintainability. By standardizing utilities and using modern C++ features, this refactor prepares the project for future growth and adherence to industry standards.

Refactored N3Utils.h to introduce various platform-agnostic string
utilities, improving code consistency and cross-platform compatibility.

Changes include:
- Added concepts for CharType, StringType, StringViewType, and PathType.
- Introduced case-insensitive comparison functions (iequals, istarts_with,
  iends_with) for strings and filesystem paths, using a custom ToLower
  functor to handle case transformations across char types.
- Defined platform-specific mappings for string functions, improving
  portability across Windows, macOS, and Linux.
- Updated comments and instructions for verifying compatibility with
  C++20 compilers (GCC, Clang, MSVC) on godbolt.org.

These improvements enhance code reuse and ensure compatibility across
multiple platforms, supporting the goal of a more robust, standardized
codebase.
@stevewgr
Copy link
Member Author

Build passing: https://github.com/ko4life-net/ko/actions/runs/11810866975

Let's wait for atleast 2 maintainers (@ko4life-net/stevemaintainers) and 1-2 @ko4life-net/steveteam members to approve this pull-request. 🚀

@stevewgr
Copy link
Member Author

Further notes:
Calling std::filesystem::path::string() on Windows returns a UTF-8 compatible encoded string, making the conversion safe for storing Unicode characters even when represented as std::string. On Unix systems, it simply returns the string without needing conversion, as strings are already represented as UTF-8 by default. On Windows, it safely performs the conversion from wide to narrow UTF-8 encoded strings.

Although this introduces minor overhead on Windows, it is preferable to using raw std::string with an assumed UTF-8 encoding, which might instead be interpreted as ASCII on Windows.

It’s also worth mentioning that calling std::filesystem::path::generic_string() returns / on all platforms, making it preferable in cases requiring consistent separators. However, it has the overhead of internally finding \ and replacing them with /, so use it only when needed. Our wrapper fs::pathx automatically normalizes paths in important cases since on Windows both \ and / are acceptable. Caution should be taken when writing file paths to disk, as we still want compatibility with the official assets of KO.

Approach for Identifying Replacements with std::filesystem

The following notes outline the methods and patterns I used to replace legacy file handling with std::filesystem. These replacements focus on enhancing cross-platform support while maintaining compatibility:

  • Abstracted GetModuleFileName calls
  • Replaced GetCurrentDirectory -> std::filesystem::current_path
  • Replaced SetCurrentDirectory -> std::filesystem::current_path
  • Replaced DeleteFile -> std::filesystem::remove
  • Replaced CreateDirectory -> std::filesystem::create_directory
  • Replaced path concatenation using sprintf -> std::filesystem::operator/
  • Replaced _makepath -> std::filesystem::operator/
  • Replaced _splitpath -> std::filesystem::path
  • Replaced CFileFind, WIN32_FIND_DATA, _finddata_t, _findfirst, _findnext -> std::filesystem::directory_iterator
  • Replaced "C:\\" -> std::filesystem::operator/
  • Replaced '\\' -> std::filesystem::operator/
  • Fixed multiple potential buffer overruns by using dynamically allocated buffers
  • Searched for instances of new CN3BaseFileAccess where paths were converted to relative paths
  • Searched for _MAX_PATH, MAX_FNAME, MAX_EXT instances and cleaned them up
  • Removed redundant CharLower calls
  • Replaced instances of strFile*, szFile*, szFN* with fs::path where applicable
  • Replaced *szPath with fs::path where applicable
    • Note: Some functions like MFC’s OnOpenDocument and OnSaveDocument are declared as virtual and were left untouched
  • Replaced CStringArray containers that stored file paths with std::vector<fs::path> where applicable
  • Avoided modifying INI and ZIP file implementations, as they will be replaced with cross-platform libraries
  • Ensured that all file path writes using fs::path maintain Windows-style backslashes for compatibility with existing file formats
    • Note: Although Windows supports both forward and back path separators, we’ll adhere to official formats to maintain compatibility with the official client editors.

Copy link
Contributor

@xGuTeK xGuTeK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s a summary of what I’ve tested:

Compilation: No issues encountered

Server startup and testing: Everything works flawlessly

Client:

  • Loading Models/Objects: All good
  • Object collisions: Functioning properly
  • Loading textures/images: No issues
  • Loading sounds: Working perfectly
  • Loading UIF files: Everything loads fine
  • Loading TBL files: No problems
  • Loading skill effects: Appears to be fine—level-up effects are working, but since skills aren't fully functional yet, I can’t test all skill effects. We can revisit this later.

Launcher: No issues
Options: No issues
N3M3: No issues
Kscviewer: No issues

This is a large PR that requires thorough testing. Even if we approve it now, additional testing will be necessary in the future. While it's possible that we may encounter issues later, ideally, they should not arise given that the changes themselves were not overly complex, though there were many of them. Amazing work @stevewgr 🚀 🚀🚀

@xGuTeK
Copy link
Contributor

xGuTeK commented Nov 13, 2024

We need to check maps with ponds and rivers

@stevewgr
Copy link
Member Author

We need to check maps with ponds and rivers

Ponds and rivers works in terms of loading. I tested with a debugger. The old maps still got some ponds, but no rivers.

Copy link
Member

@ko4life-psko ko4life-psko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @stevewgr! Unfortunately I didn’t have time to review the changes under the tool directory or run the application locally, so I focused on skimming through the code changes, and it looks good to me. 🚀

My comments are just feedback or points for your attention in case you want to further improve it, but overall, I think it’s ready to merge.

src/common/N3Utils.h Outdated Show resolved Hide resolved
src/common/N3Utils.h Show resolved Hide resolved
src/engine/N3Base/BitMapFile.cpp Show resolved Hide resolved
src/engine/N3Base/N3ShapeMod.h Outdated Show resolved Hide resolved
src/engine/N3Base/N3SndEng.cpp Outdated Show resolved Hide resolved
src/game/GameBase.cpp Show resolved Hide resolved
src/server/LoginServer/SettingDlg.cpp Outdated Show resolved Hide resolved
src/server/LoginServer/DBProcess.cpp Outdated Show resolved Hide resolved
stevewgr added a commit to ko4life-net/ko-db that referenced this pull request Nov 19, 2024
This change sets MSSQL 2019 as the minimum SQL Server version.
For older versions, the collation can be reverted to
`SQL_Latin1_General_CP1_CI_AS` by modifying the `import.ps1` script.
However, UTF-8 compatibility is required for proper functionality.

UTF-8 ensures consistent behavior regardless of the host system's
settings. On Unix, UTF-8 is typically enabled by default. On Windows,
it can be enabled under Region settings with "Beta: Use Unicode UTF-8
for worldwide language support."

For details, see this discussion:
ko4life-net/ko#260 (comment)
This commit introduces `fs::pathx`, a custom wrapper around
`std::filesystem::path`, to address limitations in the standard
implementation. This extended class provides additional functionality
and optimizations while maintaining compatibility with existing code.

Key features include:
- **Automatic Path Normalization**: FS_AUTO_NORMALIZE automatically
  standardizes path separators during concatenation with `operator/`
  or `operator/=`, simplifying path handling, especially on Windows.
- **Extended Path Manipulation**: Added functions for case-insensitive
  path comparisons (`icontains`), relative path construction, and
  converting paths to lowercase, improving flexibility in handling paths.
- **Operator Overloading**: Implemented `operator/` and `operator+=`
  to streamline path concatenation, along with other convenient
  overloads.
- **Temporary File Creation**: Added `mktemp_file`, a function for
  creating temporary files with customizable prefixes, suffixes, and
  directory locations.
- **Namespace Redirection**: The new class uses namespace redirection
  and aliasing to remain unobtrusive in the codebase, making it behave
  like a native `std::filesystem::path` object.

These enhancements offer a more versatile and efficient approach to
path management, particularly on Windows, and allow for a cleaner,
more maintainable codebase. This approach should remain effective
until future standard updates to `std::filesystem` address current
limitations.
Added a console log output in CLogWriter::Write to display log
messages directly in the console in addition to file output. This
enhancement helps improve real-time debugging by making logs
immediately visible.
@stevewgr stevewgr marked this pull request as draft November 19, 2024 21:27
@stevewgr stevewgr marked this pull request as ready for review November 19, 2024 21:27
This commit replaces numerous platform-specific WinAPI functions and
custom implementations with C++17’s std::filesystem, streamlining
and modernizing path and file management across the codebase.
The refactor enhances code readability, reduces platform-specific
dependencies, and improves cross-platform compatibility.

Summary of changes:
- **Path Handling**: Replaced custom and WinAPI-based path handling
  across files, standardizing operations like path concatenation,
  normalization, and extraction using std::filesystem methods.
- **File I/O Operations**: Migrated file reading, writing, and
  existence checks to use std::filesystem, ensuring cleaner and
  more uniform file operations across platforms.
- **Legacy Code Removal**: Removed obsolete components such as
  `CAVIPlayer` and other WinAPI-based implementations no longer
  required with std::filesystem support.
- **Bug Fixes**: Addressed existing path-related bugs and inconsistencies
  that stemmed from the manual path handling methods previously used.
- **Code Cleanup**: Deleted unnecessary code comments and unused
  functions, including those related to deprecated path handling,
  for a cleaner, more maintainable codebase.

This update provides a solid foundation for improved cross-platform
support and prepares the codebase for future C++ standard upgrades.
The `branch` option in `.gitmodules` is only relevant when tracking
remote changes to a specific branch, such as a dedicated development
branch. However, we use `master` as both the default development branch
and the tagging mechanism for releases. As we don’t rely on a complex
branching strategy, the `branch` tags are unnecessary for our use case.
Note that we released a new 1.2.0 version, as well as added updates
related to the changes in this PR where it affects how we read the
VERSION table.
Note that ko-db now includes its own submodule for archived migration
scripts. To speed-up clonning time, we don't init the submodules
recursively.
C++26 plans to make `std::filesystem::path` compatible with `std::format`,
but why wait for the future when we can build it ourselves today?

This change brings tomorrow's functionality to life. If C++26 eventually
delivers the same support, we can gracefully retire our custom solution
and let the future take over.
This update improves the `CLogWriter` class by enhancing portability and
performance:

- Replaced WinAPI's `CreateFile` with `std::fstream` for file operations,
  ensuring compatibility across platforms.
- Updated timestamp generation to use `std::chrono`, improving code clarity
  and modernizing time handling.
- Introduced a `std::mutex` for thread-safe logging to address potential
  concurrency issues.
- Simplified and standardized log messages using `std::format`, improving
  readability and maintainability.

These changes modernize the logging implementation, making it more
portable and easier to maintain while adhering to modern C++ standards.
@stevewgr stevewgr marked this pull request as draft November 19, 2024 22:04
@stevewgr stevewgr marked this pull request as ready for review November 19, 2024 22:04
@stevewgr
Copy link
Member Author

Build passing: https://github.com/ko4life-net/ko/actions/runs/11922099338

Thanks everyone for the support and reviews, you're awesome! 💥 Let's ship the ship 🚀

image

@stevewgr stevewgr merged commit d4cb1a6 into master Nov 19, 2024
1 check passed
@stevewgr stevewgr deleted the std-filesystem branch November 19, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants