Refactor: Implement Input Action Mapping System - #156
Conversation
…dings and Persistence - Introduced GameAction enum and InputMapper for hardware-agnostic input - Added support for configurable key and mouse bindings - Implemented settings persistence in src/game/settings.zig - Refactored Player, App, Session, and MapController to use the action system - Resolves #148
github-actions
Bot
added
documentation
|
/oc Check for code quality issues |
|
Failed to parse response. Part types found: [step-start, step-finish] |
Code Review: Input Action Mapping SystemCritical Issues1. Memory Leak in var input_settings = InputSettings.init(ctx.allocator);
input_settings.input_mapper = ctx.input_mapper.*;
input_settings.save() catch |err| { ... };
2. Duplicate Settings Loading in var input_mapper = InputMapper.init(); // Default init
const input_settings = InputSettings.load(allocator);
input_mapper = input_settings.input_mapper; // Immediately overwrittenThe default 3. Missing Allocator in pub fn load(allocator: std.mem.Allocator) Settings {
var settings = Settings.init(allocator);The Potential Bugs4. JSON Parsing is Fragile in
5. Missing Error Handling in const move_vec = mapper.getMovementVector(input);
if (move_vec.z > 0) move_dir = move_dir.add(self.forward);No fallback if 6. Inconsistent Error Propagation in const input_settings = InputSettings.load(allocator);
input_mapper = input_settings.input_mapper;If Code Quality Issues7. Dead Code in _ = action; // Unused variable - should be removed8. Overly Complex State Machine in 9. Magic Numbers in const SETTINGS_FILENAME = "settings.json";
const APP_NAME = "zigcraft";These should be documented or in a dedicated constants section. 10. Missing
Improvements11. Use std.json Instead of Manual JSON 12. Add Validation to 13. Document the 14. Consider Adding Reset-to-Defaults for Individual Bindings Summary
The most pressing issues are the memory leak in |
…ON parsing - Fixed memory leaks in menus.zig and app.zig by adding missing deinit calls - Optimized InputMapper initialization in app.zig to avoid redundant default init - Renamed settings.zig to input_settings.zig for clarity and added deinit() - Refactored InputMapper and InputSettings to use std.json for robust serialization - Extracted handleUiBack() in app.zig to simplify the main update loop - Updated code to use new Zig 0.16.0-dev APIs (std.Io.Writer.Allocating, ArrayList.empty) - Added documentation for all GameAction entries
|
/oc Check for code quality issues |
Code Review: Input Action Mapping SystemCritical Issues1. Memory Leak in pub fn resetAction(self: *InputMapper, action: GameAction) void {
const defaults = InputMapper.init(); // Allocates new mapper
self.bindings[@intFromEnum(action)] = defaults.bindings[@intFromEnum(action)];
// defaults is never deinitialized - memory leak
}
2. Inconsistent Error Handling in var input_settings = InputSettings.load(allocator);
defer input_settings.deinit();
const input_mapper = input_settings.input_mapper;The comment "Load custom bindings" is misleading. If 3. Fragile JSON Parsing in pub fn deserialize(self: *InputMapper, allocator: std.mem.Allocator, data: []const u8) !void {
var parsed = try std.json.parseFromSlice([GameAction.count]ActionBinding, allocator, data, .{
.ignore_unknown_fields = true,
});
defer parsed.deinit();
self.bindings = parsed.value;
}The test at lines 413-428 manually parses JSON instead of using Potential Bugs4. Dead Code in _ = action; // Unused variable - should be removedThis appears in the deserialize function but 5. Missing Validation in 6. Platform Path Handling in fn getSettingsPath(allocator: std.mem.Allocator) ![]u8 {
if (builtin.os.tag == .linux) {Missing cases for Android, FreeBSD, and other supported Zig targets will cause Code Quality Issues7. Unused Variable in var input_settings = InputSettings.load(allocator);
defer input_settings.deinit();
const input_mapper = input_settings.input_mapper;
const input_mapper = InputSettings.load(allocator).input_mapper;
defer { _ = input_mapper; } // Keep deinit8. Missing Allocator Documentation 9. Inconsistent State Machine Complexity Improvements10. Add Reset-to-Defaults for Individual Bindings pub fn resetActionToDefault(self: *InputMapper, action: GameAction) void(With static defaults array to avoid allocation - see issue #1) 11. Document Default Bindings /// Move player forward (Default: W)
move_forward,12. Consider Bidirectional Equality pub fn eql(self: InputBinding, other: InputBinding) bool {
// Also check if key == key_alt for same physical key
}Summary
The implementation is mostly solid but needs fixes for the memory leak and error handling visibility before merge. The test coverage for deserialization should be updated to actually test the |
- Fixed serialize/toJson to correctly transfer ownership from Allocating writer - Fixed memory leak in InputMapper.serialize and InputSettings.toJson - Updated InputMapper unit test to use refactored deserialize method - Resolved UnexpectedEndOfInput error caused by returning empty buffers
…nd robust testing - Replaced heap-allocating InputMapper.init() in resetAction with static DEFAULT_BINDINGS to fix memory leak - Renamed resetAction to resetActionToDefault for clarity - Fixed silent data loss on settings load by adding comprehensive logging in InputSettings.load - Updated platform path handling with robust fallbacks in getSettingsPath - Refactored InputBinding.eql to handle symmetric equality between .key and .key_alt - Updated InputMapper tests to verify actual deserialize() method and equality logic - Documented default keys for all GameAction entries - Optimized App.init to avoid redundant InputMapper copies
|
/oc Check for code quality issues |
Code Review SummaryCritical Issues1. Missing fn parseJson(self: *InputSettings, data: []const u8) !void {Should be 2. Potential undefined behavior in self.bindings = parsed.value;The High Priority Issues3. Duplicate settings initialization pattern in var input_settings = InputSettings.init(self.allocator);
defer input_settings.deinit();
input_settings.input_mapper = self.input_mapper;This creates a new 4. Same pattern in var input_settings = InputSettings.init(ctx.allocator);
defer input_settings.deinit();
input_settings.input_mapper = ctx.input_mapper.*;Duplicated code pattern - consider extracting a shared Medium Priority Issues5. Incomplete platform path detection in } else {
// Generic fallback for other platforms
return std.fmt.allocPrint(allocator, "./{s}", .{SETTINGS_FILENAME});
}Android and other mobile platforms may not have a writable current directory. Consider using app-specific storage paths where available. 6. No version migration in serialization {"version":2,"bindings":[...]}If settings schema changes in the future, there's no migration path from v1 to v2. Consider adding migration logic or at minimum a comment about backward compatibility expectations. 7. Silent data loss on settings load error in const data = std.fs.cwd().readFileAlloc(...) catch |err| {
if (err != error.FileNotFound) {
log.log.warn("...Using default bindings.", .{ path, err });
}
return settings;
};When file is corrupt, user loses custom bindings without explicit warning about what specifically failed. The current warning is good but consider adding a more specific error message. Low Priority Issues8. Magic number for max file size in const data = std.fs.cwd().readFileAlloc(path, allocator, @enumFromInt(1024 * 1024)) catch ...Consider defining 9. Test coverage gap for test "InputSettings.load handles corrupt file" {
// Write corrupt JSON, verify default bindings returned
}Suggested Improvements10. Add 11. Consider adding pub fn createFromMapper(allocator: std.mem.Allocator, mapper: InputMapper) InputSettings {
return .{ .allocator = allocator, .input_mapper = mapper };
}Would eliminate the initialization-then-overwrite pattern. Summary
The implementation is solid overall. The previous memory leak from |
…nt allocations, and improve error handling - Refactored InputMapper to use a static DEFAULT_BINDINGS array, fixing memory leaks and improving performance - Added initFromMapper and saveFromMapper helpers to InputSettings to avoid redundant default allocations - Improved platform detection in getSettingsPath (added BSD support and better fallbacks) - Added version check and migration log to InputSettings - Enhanced InputBinding.eql to be bidirectional and correctly handle key equivalents - Documented default keys for all GameActions - Added unit tests for resetActionToDefault and corrupt settings file handling
|
/oc Check for code quality issues |
|
Based on my review of the code, here are the issues I found: Critical Issues1. Missing error propagation in settings.parseJson(data) catch |err| {
log.log.warn("Failed to parse settings file...", .{});
}; // Error is swallowed - parseJson returns void on errorThis causes user-customized bindings to be lost without the caller knowing. 2. Silent data loss on settings load failure ( High Priority Issues3. Version migration is incomplete ( if (parsed.value.version < 2) {
log.log.info("Migrating input settings...");
// No actual migration logic - just accepts potentially incompatible data
}A v1 file with fewer bindings could corrupt v2 settings by overwriting newer action bindings with old data. 4. Duplicate initialization pattern in var input_settings = InputSettings.load(allocator);
// ...
const input_mapper = input_settings.input_mapper;
pub fn loadAndReturnMapper(allocator: std.mem.Allocator) InputMapper {
return InputSettings.load(allocator).input_mapper;
}Medium Priority Issues5. No validation of bindings array length ( var parsed = try std.json.parseFromSlice(Schema, ...);If the saved JSON has wrong array size, this could cause issues. Consider adding validation after parsing. 6. Test coverage gap for Code Quality ObservationsPositive patterns observed:
Minor suggestions:
|
…ings.load - Swapped allocator and path arguments - Wrapped MAX_SETTINGS_SIZE in Io.Limit.limited() - Verified fix via successful unit tests
…nitialization efficiency - Improved error propagation and logging in InputSettings.load - Implemented basic version migration and validation in parseJson - Added InputSettings.loadAndReturnMapper to avoid redundant default initialization - Used @memcpy for explicit copy semantics in binding assignments - Added unit tests for version migration and corrupt settings handling - Documented CURRENT_VERSION and MAX_SETTINGS_SIZE
…tedToken - JSON parser in different Zig versions might return either error for garbage input - Verified fix passes locally and addresses integration test failure
|
/oc Check for code quality issues |
Code Review: Input Action Mapping SystemBased on my review of the code, here's my analysis: ✅ Issues Fixed
|
| Category | Status |
|---|---|
| Memory Safety | ✅ No leaks |
| Error Handling | ✅ Proper propagation |
| API Design | ✅ Clean helpers |
| Test Coverage | ✅ Migration & corruption tests |
| Code Quality | ✅ Well-structured |
Approval: The implementation is solid. All previously identified critical issues have been resolved. The code is ready for merge.
1 participant
Summary
This PR implements a comprehensive Input Action Mapping system as requested in #148. It has been thoroughly refactored to address all code review feedback, resolve integration test failures, and improve system robustness.
Key Changes:
GameActionenum to represent player intents (e.g.,Jump,InteractPrimary) instead of hardcoded keys. Default keys are documented in the enum.InputMappernow supports runtime rebinding with support for both primary and alternate bindings (Keyboard & Mouse).std.jsonfor all settings persistence, utilizing the latest Zig 0.16.0-dev APIs.InputSettingsinsrc/game/input_settings.zigwith robust platform path detection (including BSD support) and comprehensive error logging.Player,App,Session,MapController, andCamerato depend on logical actions.loadandparseJsonto propagate errors correctly and provide detailed warnings.loadAndReturnMapperto streamline system startup.InputMapperallocations with a staticDEFAULT_BINDINGSarray.InputBinding.eqlto correctly handle.keyand.key_altrepresenting the same physical key symmetrically.Verification:
zig build.Resolves #148