TransformCtrl: Add new commands #254
Conversation
WalkthroughThe pull request introduces multiple modifications across various files, primarily enhancing keyboard input handling and transformation command management within the animation framework. Key changes include the addition of a new member variable for keyboard transformation control in Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
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: 5
🧹 Outside diff range and nitpick comments (6)
src/Engine/App/CameraComponent.cpp (1)
163-165: Consider adding error logging for debugging.The error handling change from assertion to conditional return is good, but consider adding debug logging when the service is null to aid in troubleshooting.
if (nullptr == rbSrv) { + osre_debug(Tag, "RenderBackendService is null in CameraComponent::onRender"); return false; }samples/01_ModelLoading/ModelLoading.cpp (2)
169-171: Consider adding bounds checking for key handling.The key handling could be more robust with additional validation:
Platform::Key key = AppBase::getKeyboardEventListener()->getLastKey(); -if (key != Platform::KEY_UNKNOWN) { +if (key != Platform::KEY_UNKNOWN && mKeyboardTransCtrl != nullptr) { mKeyboardTransCtrl->update(mKeyboardTransCtrl->getKeyBinding(key)); }
Line range hint
44-56: Consider extracting transformation logic to a separate component.The current implementation mixes model loading and transformation concerns in the same class. Consider creating a dedicated TransformationComponent to better encapsulate the transformation logic and improve maintainability.
This would:
- Improve separation of concerns
- Make the transformation logic reusable
- Simplify testing
src/Engine/Animation/AnimatorBase.h (1)
135-140: Consider aligning naming conventions across enum entries.The new transform commands are logically structured and provide good granular control. However, there's an inconsistency in naming patterns:
- Existing entries:
RotateXCommandPositive- New entries:
TransformCommandXPositiveConsider aligning these in a future update for better maintainability.
# Suggested future naming alignment (not for this PR): - RotateXCommandPositive = 0, - RotateXCommandNegative, + RotateCommandXPositive = 0, + RotateCommandXNegative, // ... other rotate commands ... ScaleInCommand, ScaleOutCommand, - TransformCommandXPositive, - TransformCommandXNegative, + TranslateCommandXPositive, + TranslateCommandXNegative, // ... other translate commands ...src/Engine/App/TransformController.cpp (2)
103-121: Optimize rotation handling to reduce redundant matrix creation.The rotation cases could be optimized by:
- Consolidating the rotation logic
- Avoiding redundant identity matrix creation
- glm::mat4 rot(1.0); + const glm::vec3 axes[] = { + glm::vec3(1, 0, 0), // X axis + glm::vec3(0, 1, 0), // Y axis + glm::vec3(0, 0, 1) // Z axis + }; + switch (cmdType) { case TransformCommandType::RotateXCommandPositive: - mTransform.mModel *= glm::rotate(rot, mTransformConfig.mRotateFactor, glm::vec3(1, 0, 0)); + mTransform.mModel = glm::rotate(mTransform.mModel, mTransformConfig.mRotateFactor, axes[0]); break; // ... similar changes for other rotation cases
122-139: Standardize matrix multiplication approach.There's inconsistent usage of matrix multiplication operators:
- Rotation and scaling use
*=- Translation uses direct assignment
This could lead to confusion. Consider standardizing to one approach.case TransformCommandType::TransformCommandXPositive: - mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(mTransformConfig.mTranslateFactor, 0, 0)); + mTransform.mModel *= glm::translate(glm::mat4(1.0f), glm::vec3(mTransformConfig.mTranslateFactor, 0, 0)); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
samples/01_ModelLoading/ModelLoading.cpp(5 hunks)src/Engine/Animation/AnimatorBase.h(1 hunks)src/Engine/App/CameraComponent.cpp(1 hunks)src/Engine/App/TransformController.cpp(2 hunks)src/Engine/App/TransformController.h(3 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp
🔇 Additional comments (5)
samples/01_ModelLoading/ModelLoading.cpp (2)
56-56: LGTM: Member variable properly declared and initialized.
The new member variable mKeyboardTransCtrl is correctly declared and initialized in the constructor.
Also applies to: 65-66
175-178: Verify resource cleanup.
The error handling for RenderBackendService is good, but we should verify proper cleanup of resources.
✅ Verification successful
Resource cleanup appears to be properly handled
The code shows proper resource management:
- The class uses smart pointers (
TransformComponent::NodePtr) formModelNode - The destructor is defaulted (
~ModelLoadingApp() override = default;) which is appropriate since:- Member
mCamerais owned by the Entity/World system (created viacreateComponent) mKeyboardTransCtrlis managed by the base class (obtained viagetTransformController)- Other members are simple value types or strings
- Member
The error handling at line 175-178 is safe as it's an early return without any resource allocation preceding it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for destructor or cleanup implementations
ast-grep --pattern 'ModelLoadingApp::~ModelLoadingApp() {
$$$
}'
# Search for potential memory management patterns
rg -l "delete\s+mKeyboardTransCtrl"
Length of output: 106
Script:
#!/bin/bash
# Search for destructor implementations with broader pattern
ast-grep --pattern 'ModelLoadingApp::~ModelLoadingApp'
# Search for class definition to understand member variables
ast-grep --pattern 'class ModelLoadingApp {
$$$
}'
# Look for cleanup patterns in the file
rg -A 5 "delete|release|cleanup" samples/01_ModelLoading/ModelLoading.cpp
Length of output: 187
Script:
#!/bin/bash
# Look for the ModelLoading.h file which might contain the class definition
fd ModelLoading.h
# Once found, let's examine its content
cat samples/01_ModelLoading/ModelLoading.h
# Look for any resource management in the cpp file
cat samples/01_ModelLoading/ModelLoading.cpp
Length of output: 7497
src/Engine/App/TransformController.h (1)
136-141: Initialization of TransformConfig is correctly implemented.
The TransformConfig struct is properly initialized with default values, enhancing configurability.
src/Engine/App/TransformController.cpp (2)
38-40: LGTM! The initialization is clean and the naming is improved.
The rename from mKeyboardMap to mInputMap is a good change as it makes the code more generic and potentially allows for different types of input devices in the future.
44-45: LGTM! The method is simple and consistent.
The getter method properly reflects the member variable rename and maintains a clean interface.
| if (nullptr == rbSrv) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results from all our verification attempts, I can now generate the final response:
Return value from CameraComponent::render() is not properly handled in callers
The verification shows that callers of the render method do not properly handle the boolean return value:
- In
World.cpp,mActiveCamera->render(rbSrv)is called without checking the return value - Similar unchecked render calls were found in
AppBase.cpp,Stage.cpp, andEntity.cpp - Only
RenderTestSuite.cppproperly checks the return value and handles the error case
The change to return false on null service should be accompanied by proper error handling in all calling code.
🔗 Analysis chain
Verify error handling in callers.
The change to return false on null service is good, but we should verify that callers handle this return value appropriately.
Let me analyze the results and generate additional verification scripts to get more context about error handling.
The first script will help us see the full context of camera rendering in World class. The second will show us the render method implementations to check error propagation. The third will give us more context around render calls in the App directory.
Let me fix the ripgrep command and try again.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to CameraComponent::render or onRender to verify error handling
ast-grep --pattern 'if ($component->render($service)) {
$$$
}'
# Also check for direct calls without error checking
rg -A 2 "\->render\(.*\)" --type cpp
Length of output: 2135
Script:
#!/bin/bash
# Check the World class implementation for error handling of camera render
ast-grep --pattern 'mActiveCamera->render($service)'
# Check Entity class implementation for component render error handling
ast-grep --pattern 'render($service) {
$$$
}'
# Look for any error handling patterns around render calls
rg -B 2 -A 2 "->render\(" src/Engine/App/
Length of output: 553
Script:
#!/bin/bash
# Check the World class implementation for error handling of camera render
rg -B 2 -A 2 "mActiveCamera\->render" src/Engine/App/
# Look for render method implementations
ast-grep --pattern 'bool render($service) {
$$$
}'
# Check for error handling patterns in caller code
rg -B 2 -A 2 "if.*->render" src/Engine/App/
Length of output: 363
| TransformCommandXPositive, | ||
| TransformCommandXNegative, | ||
| TransformCommandYPositive, | ||
| TransformCommandYNegative, | ||
| TransformCommandZPositive, | ||
| TransformCommandZNegative, |
There was a problem hiding this comment.
💡 Codebase verification
Z-axis transform commands are missing in the implementation
The switch statement in TransformController::update is missing cases for TransformCommandZPositive and TransformCommandZNegative, although these commands are defined in the enum. The implementation needs to be updated to handle Z-axis translations similar to X and Y axes.
Missing implementations:
case TransformCommandType::TransformCommandZPositivecase TransformCommandType::TransformCommandZNegative
🔗 Analysis chain
Verify implementation of new transform commands.
The new transform commands will need proper handling in all derived classes of AnimationControllerBase. Please ensure:
- All switch statements handling
TransformCommandTypeare updated - The
getKeyBindingimplementations map appropriate keys to new commands - The
updateimplementations handle the new transform cases
Let me try a different search strategy to find the implementations.
Let me search for the implementation files and the specific handling of transform commands.
Let me check for the Z-axis transform commands which seem to be missing in the switch statement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all potential derived classes and command handlers
echo "Searching for derived classes of AnimationControllerBase..."
ast-grep --pattern 'class $_ : $$$public$$$AnimationControllerBase$$$'
echo "Searching for TransformCommandType switch statements..."
ast-grep --pattern 'switch $_($$$TransformCommandType$$$) {
$$$
}'
Length of output: 393
Script:
#!/bin/bash
# Search for files containing AnimationControllerBase
echo "Files containing AnimationControllerBase:"
rg -l "AnimationControllerBase"
echo -e "\nSearching for switch statements with TransformCommand:"
rg -B1 -A5 "switch.*TransformCommand"
echo -e "\nSearching for getKeyBinding implementations:"
rg -B1 -A5 "getKeyBinding.*override"
echo -e "\nSearching for update method implementations:"
rg -B1 -A5 "update.*override.*TransformCommand"
Length of output: 1389
Script:
#!/bin/bash
# Look for TransformController implementation
echo "TransformController implementation:"
rg -p "TransformController::getKeyBinding" -B1 -A10
echo -e "\nSearching for TransformCommand handling in cpp files:"
fd -e cpp -x rg -p "TransformCommand" -B2 -A5 {}
echo -e "\nLooking for switch statements in TransformController:"
fd "TransformController.cpp" -x cat {}
Length of output: 10552
Script:
#!/bin/bash
# Check if there are any Z-axis transform implementations elsewhere
rg -p "TransformCommandZPositive|TransformCommandZNegative" -B2 -A2
# Look for the InputMap implementation to see key bindings
echo -e "\nSearching for InputMap implementation:"
fd -e cpp -e h -x rg -p "InputMap.*setDefault" -B2 -A10 {}
Length of output: 800
| KeyArray[Platform::KEY_s] = Animation::TransformCommandType::RotateZCommandPositive; | ||
| KeyArray[Platform::KEY_S] = Animation::TransformCommandType::RotateZCommandPositive; | ||
| KeyArray[Platform::KEY_f] = Animation::TransformCommandType::RotateZCommandNegative; | ||
| KeyArray[Platform::KEY_F] = Animation::TransformCommandType::RotateZCommandNegative; |
There was a problem hiding this comment.
Resolve conflicting key mappings for KEY_s and KEY_S.
The keys KEY_s and KEY_S are mapped to multiple commands:
- Rotation Commands (Lines 68-71):
KEY_sandKEY_Smapped toRotateZCommandPositive.
- Translation Commands (Lines 77-78):
KEY_sandKEY_Smapped toTransformCommandXNegative.
This conflict will cause ambiguity in input handling, leading to unexpected behavior when the s or S key is pressed.
To fix this issue, assign different keys to one set of commands. For example, you could change the keys for the rotation commands:
- KeyArray[Platform::KEY_s] = Animation::TransformCommandType::RotateZCommandPositive;
- KeyArray[Platform::KEY_S] = Animation::TransformCommandType::RotateZCommandPositive;
- KeyArray[Platform::KEY_f] = Animation::TransformCommandType::RotateZCommandNegative;
- KeyArray[Platform::KEY_F] = Animation::TransformCommandType::RotateZCommandNegative;
+ KeyArray[Platform::KEY_z] = Animation::TransformCommandType::RotateZCommandPositive;
+ KeyArray[Platform::KEY_Z] = Animation::TransformCommandType::RotateZCommandPositive;
+ KeyArray[Platform::KEY_x] = Animation::TransformCommandType::RotateZCommandNegative;
+ KeyArray[Platform::KEY_X] = Animation::TransformCommandType::RotateZCommandNegative;Alternatively, adjust the keys for the translation commands to resolve the conflict.
Also applies to: 77-78
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/Engine/App/TransformController.h (2)
136-141: Consider enhancing the TransformConfig struct.While the default values are reasonable, consider these improvements:
- Add documentation for each factor explaining its purpose and impact
- Consider making these configurable through constructor parameters
- Add validation for factor ranges (e.g., scale factors should be positive)
struct TransformConfig { + /// @brief Scale factor for increasing size (must be > 1.0) f32 mScaleFactorPlus = 1.01f; + /// @brief Scale factor for decreasing size (must be < 1.0) f32 mScaleFactorMinus = 0.99f; + /// @brief Rotation step in radians f32 mRotateFactor = 0.01f; + /// @brief Translation step in world units f32 mTranslateFactor = 0.15f; + + TransformConfig(f32 scalePlus = 1.01f, f32 scaleMinus = 0.99f, + f32 rotate = 0.01f, f32 translate = 0.15f) : + mScaleFactorPlus(scalePlus), + mScaleFactorMinus(scaleMinus), + mRotateFactor(rotate), + mTranslateFactor(translate) { + assert(mScaleFactorPlus > 1.0f && "Scale plus factor must be > 1.0"); + assert(mScaleFactorMinus < 1.0f && "Scale minus factor must be < 1.0"); + } };
Line range hint
115-119: Consider dependency injection for TransformConfig.The TransformController could benefit from accepting the TransformConfig as a constructor parameter, allowing for more flexible configuration management and easier testing.
class OSRE_EXPORT TransformController : public Animation::AnimationControllerBase { public: - TransformController(RenderBackend::TransformMatrixBlock &tmb); + TransformController(RenderBackend::TransformMatrixBlock &tmb, + const TransformConfig &config = TransformConfig());samples/01_ModelLoading/ModelLoading.cpp (3)
44-49: Enhance class documentation to include transformation capabilities.While the documentation explains the model loading functionality, it should also mention the newly added transformation control capabilities to provide a complete overview of the class's features.
/// @ingroup Samples /// -/// @brief This sample shows how to load a model using the Assimp wrapper. +/// @brief This sample demonstrates model loading and transformation control capabilities. /// /// It will also show some statistics about the loaded model. The model will get loaded, the -/// camera will be placed to get an optimal view onto the model. The model will be rendered. +/// camera will be placed to get an optimal view onto the model. The model can be transformed +/// using keyboard controls and will be rendered.
175-176: Enhance const correctness in keyboard input handling.Consider making the key binding retrieval const-correct to better express intent and prevent accidental modifications.
-if (Platform::Key key = AppBase::getKeyboardEventListener()->getLastKey(); key != Platform::KEY_UNKNOWN) { - mKeyboardTransCtrl->update(mKeyboardTransCtrl->getKeyBinding(key)); +if (const Platform::Key key = AppBase::getKeyboardEventListener()->getLastKey(); key != Platform::KEY_UNKNOWN) { + mKeyboardTransCtrl->update(mKeyboardTransCtrl->getKeyBinding(key));
180-183: Enhance error message specificity.The error message could be more specific about the context in which the RenderBackendService is unavailable.
- osre_error(Tag, "RenderBackendService not available."); + osre_error(Tag, "RenderBackendService not available during model transformation update.");src/Engine/App/TransformController.cpp (2)
37-38: Consider prefixing the Tag constant with the namespace.To prevent potential naming conflicts, consider prefixing the Tag constant:
-static constexpr c8 Tag[] = "TransformController"; +static constexpr c8 OSRE_TransformController_Tag[] = "TransformController";
142-145: Document why Z-axis transformations are ignored.The code silently ignores Z-axis transformations. Consider adding a comment explaining why these transformations are ignored or implement them if they should be supported.
case TransformCommandType::TransformCommandZPositive: case TransformCommandType::TransformCommandZNegative: - // ignored + // Z-axis transformations are ignored because [reason] break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
samples/01_ModelLoading/ModelLoading.cpp(3 hunks)src/Engine/App/TransformController.cpp(2 hunks)src/Engine/App/TransformController.h(3 hunks)
🔇 Additional comments (8)
src/Engine/App/TransformController.h (2)
142-143: LGTM!
The member variable changes are clean and consistent with the structural changes.
68-71:
Resolve conflicting key mappings for KEY_s and KEY_S.
The same issue with conflicting key mappings persists:
- Lines 68-69:
KEY_s/Smapped toRotateZCommandPositive - Lines 77-78:
KEY_s/Smapped toTransformCommandXNegative
This will cause ambiguous behavior when these keys are pressed.
Also applies to: 77-78
samples/01_ModelLoading/ModelLoading.cpp (3)
66-67: LGTM!
The constructor initialization follows best practices by initializing all members in the initialization list.
119-131: Well-implemented error handling for transform controller initialization.
The implementation properly handles potential initialization failures and includes appropriate error logging.
175-176: Verify transformation command handling coverage.
Let's ensure all possible transformation commands are properly handled by the controller.
✅ Verification successful
All transformation commands are properly handled
The verification shows comprehensive handling of transformation commands:
- All transformation command types are defined in
TransformCommandTypeenum - The
TransformController::updatemethod handles all command types:- Rotation commands (X, Y, Z axes, both positive and negative)
- Scale commands (in/out)
- Translation commands (X, Y axes, both positive and negative)
- Z-axis translation commands are explicitly ignored
- Key bindings are properly mapped in the
InputMapclass with default mappings for all supported commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for transformation command handling coverage
# Search for transformation command definitions and their handling
ast-grep --pattern 'enum $_ {
$$$
Transform$_
$$$
}'
# Search for command handling implementation
ast-grep --pattern 'update($_) {
$$$
case $_:
$$$
}'
Length of output: 305
Script:
#!/bin/bash
# Let's try with ripgrep to search for transformation-related code
rg -A 5 "enum.*Command|Transform.*Command" --type cpp
# Search for update method implementations
rg -A 10 "void.*update.*Command" --type cpp
# Search for key bindings related to transformations
rg "getKeyBinding|setKeyBinding" --type cpp
Length of output: 16182
src/Engine/App/TransformController.cpp (3)
41-41: LGTM! Good rename from keyboard-specific to generic input mapping.
The change from mKeyboardMap to mInputMap makes the code more generic and future-proof, allowing for different types of input devices.
Also applies to: 45-45
123-128: Consider bounds checking for scale factors.
The scale operations might benefit from bounds checking to prevent extreme scaling values that could cause rendering issues.
104-122: 🛠️ Refactor suggestion
Inconsistent matrix multiplication and unused rotation matrix.
- The rotation commands use
*=operator while some other commands use*. Consider using consistent operators. - The
rotmatrix is created at the start but unused in some transformations.
- glm::mat4 rot(1.0);
switch (cmdType) {
case TransformCommandType::RotateXCommandPositive:
- mTransform.mModel *= glm::rotate(rot, mTransformConfig.mRotateFactor, glm::vec3(1, 0, 0));
+ mTransform.mModel = glm::rotate(mTransform.mModel, mTransformConfig.mRotateFactor, glm::vec3(1, 0, 0));Likely invalid or redundant comment.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Engine/App/TransformController.cpp (3)
45-45: Consider adding error handling for invalid keysThe
getKeyBindingmethod should handle cases where the key might not exist in the map.TransformCommandType TransformController::getKeyBinding(Key key) const { + if (!mInputMap.contains(key)) { + return TransformCommandType::None; // Or appropriate default/error value + } return mInputMap.get(key); }
142-145: Consider documenting Z-axis transformation behaviorThe code silently ignores Z-axis transformations. This should be documented to explain why these operations are ignored.
case TransformCommandType::TransformCommandZPositive: case TransformCommandType::TransformCommandZNegative: - // ignored + // Z-axis transformations are intentionally ignored to prevent [reason] break;
104-149: Add validation for transformation factorsConsider adding validation for the transformation factors to prevent potential numerical issues.
Suggestions:
- Add range checks for scale factors to prevent zero or negative scaling
- Consider adding maximum limits for translation and rotation factors
- Add assertions or runtime checks for NaN/Inf values
Example validation method:
bool TransformController::validateTransformConfig() const { // Prevent zero or negative scaling if (mTransformConfig.mScaleFactorPlus <= 0.0f || mTransformConfig.mScaleFactorMinus <= 0.0f) { osre_warn(Tag, "Invalid scale factors"); return false; } // Add other validations as needed return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Engine/App/TransformController.cpp(2 hunks)
🔇 Additional comments (3)
src/Engine/App/TransformController.cpp (3)
37-38: LGTM: Tag constant follows best practices
The Tag constant is properly defined using constexpr and follows the class naming convention.
147-149: 🛠️ Refactor suggestion
Enhance error logging in default case
The warning message should include the invalid command type for better debugging.
default:
- osre_warn(Tag, "Invalid tag");
+ osre_warn(Tag, "Invalid transform command type: %d", static_cast<int>(cmdType));
break;Likely invalid or redundant comment.
129-140:
Inconsistent matrix multiplication operators
The translation operations use direct assignment (=) while rotation and scaling use compound assignment (*=). This inconsistency could lead to unexpected behavior.
- mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(mTransformConfig.mTranslateFactor, 0, 0));
+ mTransform.mModel *= glm::translate(rot, glm::vec3(mTransformConfig.mTranslateFactor, 0, 0));Apply similar changes to other translation operations.
Likely invalid or redundant comment.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
KeyboardMaptoInputMap, improving clarity and organization of input mappings.These updates enhance user experience by streamlining controls, improving rendering accuracy, and providing better error management.