Introduce and integrate keymap #224
Conversation
WalkthroughThe recent updates significantly improve keyboard input handling in the application by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant TransformController
participant KeyboardMap
User->>App: Press Key
App->>TransformController: onUpdate()
TransformController->>KeyboardMap: getKeyBinding(Key)
KeyboardMap-->>TransformController: Return TransformCommandType
TransformController-->>App: Return Command
App-->>User: Execute Command
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- contrib/assimp (1 hunks)
- contrib/cppcore (1 hunks)
- contrib/vcpkg (1 hunks)
- samples/00_HelloWorld/HelloWorld.cpp (1 hunks)
- samples/04_terrain/TerrainRendering.cpp (1 hunks)
- src/Engine/Animation/AnimatorBase.h (4 hunks)
- src/Engine/App/TransformController.cpp (1 hunks)
- src/Engine/App/TransformController.h (4 hunks)
- src/Engine/Platform/KeyTypes.h (1 hunks)
Files skipped from review due to trivial changes (3)
- contrib/assimp
- contrib/cppcore
- contrib/vcpkg
Additional comments not posted (9)
src/Engine/Animation/AnimatorBase.h (2)
Line range hint
118-128: LGTM! Improved enum structure.The addition of
InvalidandCountentries enhances error handling and command enumeration.
147-150: LGTM! New method for key binding retrieval.The
getKeyBindingmethod enhances the class's functionality by allowing retrieval of command codes based on key bindings.src/Engine/App/TransformController.h (2)
39-76: LGTM! Struct for key binding management.The
KeyboardMapstruct effectively encapsulates key bindings and enhances modularity and maintainability.
97-97: LGTM! Instance-specific key binding management.Changing
getKeyBindingto a non-static method allows for instance-specific key binding management.samples/00_HelloWorld/HelloWorld.cpp (1)
108-108: LGTM! Instance-specific key binding retrieval.Using
mKeyboardTransCtrl->getKeyBinding(key)allows for more dynamic or instance-specific behavior in response to keyboard events.src/Engine/Platform/KeyTypes.h (1)
33-33: Verify the impact ofKEY_UNKNOWNvalue change.Changing
KEY_UNKNOWNfrom0to-1may affect how unknown keys are handled. Ensure that this change does not introduce unintended behavior in the application.src/Engine/App/TransformController.cpp (2)
38-40: LGTM! Improved keyboard map initialization.The constructor now initializes
mKeyboardMapand sets its default state, enhancing the setup of keyboard mappings.
43-44: LGTM! Simplified key binding retrieval.The
getKeyBindingmethod now usesmKeyboardMapfor key retrieval, centralizing the key-command mapping logic.samples/04_terrain/TerrainRendering.cpp (1)
189-189: Encapsulation Improvement in Key Binding Retrieval.The change to use
mKeyboardTransCtrl->getKeyBinding(key)enhances encapsulation by retrieving key bindings through the instance's method. This likely improves modularity and maintainability by allowing context-specific behavior for key binding management.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
src/Engine/App/TransformController.h (1)
83-88: Clarify comment typo ingetmethod.The comment for the
getmethod contains a typo: "actieve" should be "active".- /// @brief Get the actieve mapping. + /// @brief Get the active mapping.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Engine/App/TransformController.h (4 hunks)
Additional comments not posted (2)
src/Engine/App/TransformController.h (2)
52-56: Ensure the array size matches the key range.The
initmethod initializes the array withAnimation::TransformCommandType::Invalid. Ensure thatPlatform::KEY_LASTaccurately represents the total number of keys to avoid out-of-bounds errors.Verification successful
Verification Successful: Array size matches key range.
The
Platform::KEY_LASTis correctly used to represent the total number of keys, as it follows the last enumerated key (KEY_UNDO = 322) in the enumeration. This ensures the array size matches the key range, preventing out-of-bounds errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that Platform::KEY_LAST matches the total number of keys defined. # Test: Check the definition of Platform::KEY_LAST. rg --type cpp --word-regexp 'KEY_LAST'Length of output: 452
Script:
#!/bin/bash # Description: Locate the definition of Platform::KEY_LAST in KeyTypes.h. # Test: Find the definition of KEY_LAST in the suspected file. rg --type cpp --word-regexp 'KEY_LAST' -A 5 src/Engine/Platform/KeyTypes.hLength of output: 135
Script:
#!/bin/bash # Description: Locate the complete context of Platform::KEY_LAST definition in KeyTypes.h. # Test: Find the definition of KEY_LAST with more context lines before it. rg --type cpp --word-regexp 'KEY_LAST' -B 10 src/Engine/Platform/KeyTypes.hLength of output: 373
111-111: EnsuregetKeyBindingmethod consistency.The
getKeyBindingmethod is now non-static and overrides a base class method. Ensure that all instances ofTransformControllerare correctly instantiated and that this change doesn't break existing functionality.Verification successful
getKeyBindingMethod Change is ConsistentThe
getKeyBindingmethod's change to a non-static version is consistent with the codebase, asTransformControlleris instantiated throughgetTransformControllerinAppBase.cpp. This ensures that instances are correctly created and used throughout the codebase.
AppBase.cpp: InstantiatesTransformControllerusinggetTransformController.HelloWorld.cpp,TerrainRendering.cpp,OsreEdApp.cpp: Use thegetTransformControllermethod to obtain instances.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all instances of TransformController are correctly instantiated. # Test: Search for TransformController instantiations. rg --type cpp 'TransformController\s+\w+\s*\('Length of output: 48
Script:
#!/bin/bash # Description: Broaden the search for TransformController instantiations, including header files and potential indirect instantiation patterns. # Search for TransformController instantiations in both source and header files. rg --type-add 'cpp-header:*.{h,hpp}' --type cpp --type cpp-header 'TransformController' # Search for any factory methods or patterns that might instantiate TransformController. rg --type cpp 'create.*TransformController'Length of output: 193
Script:
#!/bin/bash # Description: Correct the syntax error and search for TransformController instantiations across source and header files. # Corrected search for TransformController instantiations in both source and header files. rg --type-add 'cpp:*.{cpp,h,hpp}' 'TransformController' # Re-run the search for any factory methods or patterns that might instantiate TransformController. rg --type cpp 'create.*TransformController'Length of output: 2082
Fix review findings.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Engine/App/TransformController.h (4 hunks)
Additional comments not posted (4)
src/Engine/App/TransformController.h (4)
39-41: Existing Comment: Consider using a more dynamic container for key mappings.The use of
cppcore::TStaticArraylimits flexibility in handling key mappings. Consider using a more dynamic container likestd::unordered_mapfor easier management and potential future extensions.
58-74: Existing Comment: Consolidate key-command mappings.The
setDefaultmethod duplicates mappings for uppercase and lowercase keys. Consider using a helper function to reduce redundancy and improve readability.
79-86: Existing Comment: Validate key input insetmethod.The
setmethod directly assigns values toKeyArraywithout validation. Consider adding checks to ensure the key is within valid bounds.
127-127: Existing Comment: Consider encapsulatingKeyboardMapinitialization.The
mKeyboardMapmember is directly instantiated. Consider initializing it with a default key mapping within the constructor to ensure consistent behavior across instances.
Fix the build
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Engine/App/TransformController.h (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Engine/App/TransformController.h
Summary by CodeRabbit
New Features
KeyboardMapstruct for key bindings.Invalidstate to better represent command validation.Improvements
Bug Fixes