Conversation
- Replace stbi_write_png_to_mem with stbi_write_png_to_func in spratunpack_command.cpp to avoid "undeclared identifier" errors in some stb_image_write.h versions. - Reorder CMakeLists.txt to detect dependencies (libarchive, gettext, zopflipng) before defining targets. This fixes "archive.h not found" errors on macOS where headers are in non-standard brew paths. - Remove redundant source file inclusion in executable targets by linking to spratcore library.
- Replace stbi_write_png_to_mem with stbi_write_png_to_func in spratunpack_command.cpp for better portability and fix build errors. - Remove redundant extern "C" around stb_image_write.h include. - Reorder CMakeLists.txt to ensure dependencies (libarchive, gettext, zopflipng) are detected before target definition, fixing macOS build failures. - Optimize build by removing redundant source inclusion in executable targets. - Bump version to v0.2.7 in VERSION file to resolve immutable release error in GitHub Actions.
- Exclude temporary documentation snapshots in .gitignore - Add unit tests for compare_natural() function covering: natural number sorting, mixed alphanumeric, edge cases - All 14 tests passing (2 unit + 12 integration)
- Add -DBUILD_TESTING=ON to CMake configure steps in both jobs - Update ctest invocations to emit JUnit XML test results - Add artifact upload steps for test results (7-day retention) - Add artifact retention policy (30 days for packages) - Add dependency caching for Linux apt packages and macOS Homebrew - Update Linux apt-get to use --no-upgrade flag for cached packages
- Implement --deduplicate flag with FNV-1a content hashing - Implement --gpu-compress dxt1|dxt5 for GPU-native texture formats - Implement --dilate N for color bleeding to prevent GPU filtering artifacts - Add libsquish library integration (optional dependency) - Bump cache format version (2 → 3) - Update CMakeLists.txt with squish detection
Release v0.3.0 includes the new --deduplicate feature with support for: - Exact deduplication (byte-for-byte identical images) - Perceptual deduplication (visually similar images using dHash) - Full cache management and downstream compatibility Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Enhance spratlayout command to support three deduplication modes instead of a boolean flag: - none: No deduplication (default) - exact: Hash-detect byte-for-byte identical sprites (FNV-1a) - perceptual: Detect visually similar sprites (dHash algorithm) Changes: - Updated --deduplicate argument to accept mode value - Modified cache signature to include full mode string - Added validation for mode values with helpful error messages - Updated function signatures to pass mode string instead of boolean This enables the sprat-gui UI to properly use the deduplication modes as expected by its settings UI. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The spratcore static library includes command implementations that use libarchive functions, but wasn't linking against libarchive. This caused unresolved symbol linker errors on Windows when executables (spratlayout, spratpack, spratunpack) tried to link against spratcore. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
PR Summary
|
There was a problem hiding this comment.
Code Review
This pull request introduces hardware texture compression (DXT via libsquish), color dilation for artifact reduction, and a framework for sprite deduplication. It also includes significant CMake refactoring for dependency management and updates to the documentation and versioning. Review feedback highlights that the deduplication feature is currently incomplete, identifies a typo in the CLI usage text, and suggests a more robust structure for conditional compilation logic in the packing command.
| // Placeholder aliases vector (deduplication not yet fully implemented) | ||
| const std::vector<std::pair<std::string, std::string>> layout_aliases; |
There was a problem hiding this comment.
The --deduplicate feature is advertised in the README.md and a command-line option has been added, but the implementation appears to be incomplete. The layout_aliases vector is always empty, and the hashing logic is not yet integrated, which means no deduplication will occur. This could be misleading for users.
Please consider either completing the feature implementation or removing the option and documentation until it is fully functional.
| << tr(" --atlas-index N Pick a specific atlas index to output\n") | ||
| << tr(" --extrude N Repeat edge pixels N times (overrides layout)\n") | ||
| << tr(" --dilate N Bleed opaque pixels into transparent neighbors (N passes)\n") | ||
| << tr(" --gpu-compress FORMAT Compress to DCS: dxt1 or dxt5 (requires libsquish)\n") |
There was a problem hiding this comment.
There appears to be a typo in the usage string. "DCS" should likely be "DDS" to refer to the DirectDraw Surface file format.
| << tr(" --gpu-compress FORMAT Compress to DCS: dxt1 or dxt5 (requires libsquish)\n") | |
| << tr(" --gpu-compress FORMAT Compress to DDS: dxt1 or dxt5 (requires libsquish)\n") |
| #ifdef SPRAT_HAS_SQUISH | ||
| if (has_gpu_compress) { | ||
| output_data = compress_to_dds(atlas_data, atlas_width, atlas_height, gpu_compress_format); | ||
| if (output_data.empty()) { | ||
| std::cerr << tr("Error: Failed to compress to DDS for atlas ") << atlas_idx << tr(" (atlas dimensions must be multiple of 4)\n"); | ||
| return 1; | ||
| } | ||
| output_extension = ".dds"; | ||
| } else | ||
| #endif | ||
| { |
There was a problem hiding this comment.
The preprocessor logic for handling --gpu-compress is confusing and brittle. Using an #else that is conditionally compiled based on a separate #if is unconventional and can be difficult to maintain. While it currently works because of how has_gpu_compress is handled, a small change could easily break it.
A more robust and readable approach would be to use a more explicit structure. For example:
bool is_dds = false;
#ifdef SPRAT_HAS_SQUISH
if (has_gpu_compress) {
// ... DDS compression logic ...
is_dds = true;
}
#endif
if (!is_dds) {
// PNG generation logic
}This would make the code's intent clearer and less prone to accidental breakage.
No description provided.