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.

Sign up for GitHub

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

TransformCtrl: Add new commands #254

Merged
merged 4 commits into from
Dec 1, 2024
Merged

TransformCtrl: Add new commands #254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
50ea125
TransformCtrl: Add new commands
kimkulling Dec 1, 2024
284d2b0
Fix: Review findings
kimkulling Dec 1, 2024
759ba5a
Fix: Review findings
kimkulling Dec 1, 2024
59959c5
Fix: Review findings
kimkulling Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions samples/01_ModelLoading/ModelLoading.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ using namespace ::OSRE::RenderBackend;
static constexpr c8 Tag[] = "ModelLoadingApp";

//-------------------------------------------------------------------------------------------------
/// @ingroup Editor
/// @ingroup Samples
///
/// @brief
//-------------------------------------------------------------------------------------------------
Expand All @@ -53,6 +53,7 @@ class ModelLoadingApp : public App::AppBase {
TransformMatrixBlock mTransformMatrix; ///< The transform block.
TransformComponent::NodePtr mModelNode; ///< The mode node.
int mIntention;
Animation::AnimationControllerBase *mKeyboardTransCtrl;

public:
ModelLoadingApp(int argc, char *argv[]) :
Expand All @@ -61,7 +62,8 @@ class ModelLoadingApp : public App::AppBase {
mCamera(nullptr),
mTransformMatrix(),
mModelNode(),
mIntention(0) {
mIntention(0),
mKeyboardTransCtrl(nullptr) {
// empty
}

Expand Down Expand Up @@ -113,6 +115,15 @@ class ModelLoadingApp : public App::AppBase {
}

protected:
bool onCreate() override {
if (!AppBase::onCreate()) {
return false;
}
mKeyboardTransCtrl = AppBase::getTransformController(mTransformMatrix);

return true;
}
kimkulling marked this conversation as resolved.
Show resolved Hide resolved

void loadAsset(const IO::Uri &modelLoc) {
AssimpWrapper assimpWrapper(*getIdContainer(), getStage()->getActiveWorld(0));
if (!assimpWrapper.importAsset(modelLoc, 0)) {
Expand Down Expand Up @@ -155,23 +166,16 @@ class ModelLoadingApp : public App::AppBase {
}
}

glm::mat4 rot(1.0);
if (AppBase::isKeyPressed(Platform::KEY_A)) {
mTransformMatrix.mModel *= glm::rotate(rot, 0.01f, glm::vec3(1, 0, 0));

}
if (AppBase::isKeyPressed(Platform::KEY_S)) {
mTransformMatrix.mModel *= glm::rotate(rot, -0.01f, glm::vec3(1, 0, 0));
Platform::Key key = AppBase::getKeyboardEventListener()->getLastKey();
if (key != Platform::KEY_UNKNOWN) {
mKeyboardTransCtrl->update(mKeyboardTransCtrl->getKeyBinding(key));
}

if (AppBase::isKeyPressed(Platform::KEY_W)) {
mTransformMatrix.mModel *= glm::rotate(rot, 0.01f, glm::vec3(0, 1, 0));
}

if (AppBase::isKeyPressed(Platform::KEY_D)) {
mTransformMatrix.mModel *= glm::rotate(rot, -0.01f, glm::vec3(0, 1, 0));
}
RenderBackendService *rbSrv = ServiceProvider::getService<RenderBackendService>(ServiceType::RenderService);
if (nullptr == rbSrv) {
osre_error(Tag, "RenderBackendService not available.");
return;
}

rbSrv->beginPass(RenderPass::getPassNameById(RenderPassId));
rbSrv->beginRenderBatch("b1");
Expand Down
7 changes: 6 additions & 1 deletion src/Engine/Animation/AnimatorBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@ enum class TransformCommandType {
RotateZCommandNegative,
ScaleInCommand,
ScaleOutCommand,
TransformCommand,
TransformCommandXPositive,
TransformCommandXNegative,
TransformCommandYPositive,
TransformCommandYNegative,
TransformCommandZPositive,
TransformCommandZNegative,
Comment on lines +135 to +140
Copy link

Choose a reason for hiding this comment

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

💡 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::TransformCommandZPositive
  • case 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:

  1. All switch statements handling TransformCommandType are updated
  2. The getKeyBinding implementations map appropriate keys to new commands
  3. The update implementations 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

Count
};

Expand Down
4 changes: 3 additions & 1 deletion src/Engine/App/CameraComponent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ bool CameraComponent::onUpdate(Time) {
}

bool CameraComponent::onRender(RenderBackendService *rbSrv) {
osre_assert(nullptr != rbSrv);
if (nullptr == rbSrv) {
return false;
}
Comment on lines +163 to +165
Copy link

Choose a reason for hiding this comment

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

💡 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, and Entity.cpp
  • Only RenderTestSuite.cpp properly 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


rbSrv->setMatrix(MatrixType::View, mView);
rbSrv->setMatrix(MatrixType::Projection, mProjection);
Expand Down
74 changes: 40 additions & 34 deletions src/Engine/App/TransformController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@
using namespace ::OSRE::Platform;

TransformController::TransformController(TransformMatrixBlock &tmb) :
mKeyboardMap(),
mInputMap(),
mTransform(tmb) {
mKeyboardMap.setDefault();
mInputMap.setDefault();
}

TransformCommandType TransformController::getKeyBinding(Key key) const {
return mKeyboardMap.get(key);
return mInputMap.get(key);
}

void mapToSphere(const glm::vec2 &newPt, glm::vec3 *newVector, f32 w, f32 h, f32 r) {
Expand Down Expand Up @@ -100,38 +100,44 @@

void TransformController::update(TransformCommandType cmdType) {
glm::mat4 rot(1.0);
if (cmdType == TransformCommandType::RotateXCommandPositive) {
mTransform.mModel *= glm::rotate(rot, 0.01f, glm::vec3(1, 0, 0));
}

if (cmdType == TransformCommandType::RotateXCommandNegative) {
mTransform.mModel *= glm::rotate(rot, -0.01f, glm::vec3(1, 0, 0));
}

if (cmdType == TransformCommandType::RotateYCommandPositive) {
mTransform.mModel *= glm::rotate(rot, 0.01f, glm::vec3(0, 1, 0));
}

if (cmdType == TransformCommandType::RotateYCommandNegative) {
mTransform.mModel *= glm::rotate(rot, -0.01f, glm::vec3(0, 1, 0));
}

if (cmdType == TransformCommandType::RotateZCommandNegative) {
mTransform.mModel *= glm::rotate(rot, -0.01f, glm::vec3(0, 0, 1));
}

if (cmdType == TransformCommandType::RotateZCommandPositive) {
mTransform.mModel *= glm::rotate(rot, 0.01f, glm::vec3(0, 0, 1));
}

glm::mat4 scale(1.0);
if (cmdType == TransformCommandType::ScaleInCommand) {
mTransform.mModel *= glm::scale(scale, glm::vec3(1.01, 1.01, 1.01));
}

if (cmdType == TransformCommandType::ScaleOutCommand) {
mTransform.mModel *= glm::scale(scale, glm::vec3(0.99, 0.99, 0.99));
switch (cmdType) {
case TransformCommandType::RotateXCommandPositive:
mTransform.mModel *= glm::rotate(rot, mTransformConfig.mRotateFactor, glm::vec3(1, 0, 0));
break;
case TransformCommandType::RotateXCommandNegative:
mTransform.mModel *= glm::rotate(rot, -mTransformConfig.mRotateFactor, glm::vec3(1, 0, 0));
break;
case TransformCommandType::RotateYCommandPositive:
mTransform.mModel *= glm::rotate(rot, mTransformConfig.mRotateFactor, glm::vec3(0, 1, 0));
break;
case TransformCommandType::RotateYCommandNegative:
mTransform.mModel *= glm::rotate(rot, -mTransformConfig.mRotateFactor, glm::vec3(0, 1, 0));
break;
case TransformCommandType::RotateZCommandNegative:
mTransform.mModel *= glm::rotate(rot, -mTransformConfig.mRotateFactor, glm::vec3(0, 0, 1));
break;
case TransformCommandType::RotateZCommandPositive:
mTransform.mModel *= glm::rotate(rot, mTransformConfig.mRotateFactor, glm::vec3(0, 0, 1));
break;
case TransformCommandType::ScaleInCommand:
mTransform.mModel *= glm::scale(rot, glm::vec3(mTransformConfig.mScaleFactorPlus, mTransformConfig.mScaleFactorPlus, mTransformConfig.mScaleFactorPlus));
break;
case TransformCommandType::ScaleOutCommand:
mTransform.mModel *= glm::scale(rot, glm::vec3(mTransformConfig.mScaleFactorMinus, mTransformConfig.mScaleFactorMinus, mTransformConfig.mScaleFactorMinus));
break;
case TransformCommandType::TransformCommandXPositive:
mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(mTransformConfig.mTranslateFactor, 0, 0));
break;
case TransformCommandType::TransformCommandXNegative:
mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(-mTransformConfig.mTranslateFactor, 0, 0));
break;
case TransformCommandType::TransformCommandYPositive:
mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(0, mTransformConfig.mTranslateFactor, 0));
break;
case TransformCommandType::TransformCommandYNegative:
mTransform.mModel = glm::translate(mTransform.mModel, glm::vec3(0, -mTransformConfig.mTranslateFactor, 0));
break;
kimkulling marked this conversation as resolved.
Show resolved Hide resolved
}

Check warning

Code scanning / CodeQL

Missing enum case in switch Warning

Switch statement does not have a case for
Invalid
Loading
.
Switch statement does not have a case for
TransformCommandZPositive
Loading
.
Switch statement does not have a case for
TransformCommandZNegative
Loading
.
Switch statement does not have a case for
Count
Loading
.
}

} // namespace App
Expand Down
49 changes: 33 additions & 16 deletions src/Engine/App/TransformController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ namespace RenderBackend {
namespace App {

/// @brief The class to manage the keyboard mappings for transform commands.
struct KeyboardMap {
struct InputMap {
cppcore::TStaticArray<Animation::TransformCommandType, Platform::KEY_LAST> KeyArray;

/// @brief The class constructor.
KeyboardMap() {
InputMap() {
init();
}

/// @brief The class destructor.
~KeyboardMap() = default;
~InputMap() = default;

/// @brief Will initialize the array, no mapping is stored.
void init() {
Expand All @@ -57,20 +57,31 @@ struct KeyboardMap {

/// @brief Enables the default mapping.
void setDefault() {
KeyArray[Platform::KEY_A] = Animation::TransformCommandType::RotateXCommandPositive;
KeyArray[Platform::KEY_a] = Animation::TransformCommandType::RotateXCommandPositive;
KeyArray[Platform::KEY_D] = Animation::TransformCommandType::RotateXCommandNegative;
KeyArray[Platform::KEY_d] = Animation::TransformCommandType::RotateXCommandNegative;
KeyArray[Platform::KEY_W] = Animation::TransformCommandType::RotateYCommandPositive;
KeyArray[Platform::KEY_w] = Animation::TransformCommandType::RotateYCommandPositive;
KeyArray[Platform::KEY_S] = Animation::TransformCommandType::RotateYCommandNegative;
KeyArray[Platform::KEY_s] = Animation::TransformCommandType::RotateYCommandNegative;
KeyArray[Platform::KEY_Q] = Animation::TransformCommandType::RotateZCommandPositive;
KeyArray[Platform::KEY_q] = Animation::TransformCommandType::RotateZCommandPositive;
KeyArray[Platform::KEY_E] = Animation::TransformCommandType::RotateZCommandNegative;
KeyArray[Platform::KEY_e] = Animation::TransformCommandType::RotateZCommandNegative;
KeyArray[Platform::KEY_Q] = Animation::TransformCommandType::RotateXCommandPositive;
KeyArray[Platform::KEY_q] = Animation::TransformCommandType::RotateXCommandPositive;
KeyArray[Platform::KEY_E] = Animation::TransformCommandType::RotateXCommandNegative;
KeyArray[Platform::KEY_e] = Animation::TransformCommandType::RotateXCommandNegative;
KeyArray[Platform::KEY_y] = Animation::TransformCommandType::RotateYCommandPositive;
KeyArray[Platform::KEY_Y] = Animation::TransformCommandType::RotateYCommandPositive;
KeyArray[Platform::KEY_c] = Animation::TransformCommandType::RotateYCommandNegative;
KeyArray[Platform::KEY_C] = Animation::TransformCommandType::RotateYCommandNegative;
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;
Comment on lines +68 to +71
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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_s and KEY_S mapped to RotateZCommandPositive.
  • Translation Commands (Lines 77-78):
    • KEY_s and KEY_S mapped to TransformCommandXNegative.

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

KeyArray[Platform::KEY_PLUS] = Animation::TransformCommandType::ScaleInCommand;
KeyArray[Platform::KEY_MINUS] = Animation::TransformCommandType::ScaleOutCommand;

KeyArray[Platform::KEY_w] = Animation::TransformCommandType::TransformCommandXPositive;
KeyArray[Platform::KEY_W] = Animation::TransformCommandType::TransformCommandXPositive;
KeyArray[Platform::KEY_s] = Animation::TransformCommandType::TransformCommandXNegative;
KeyArray[Platform::KEY_S] = Animation::TransformCommandType::TransformCommandXNegative;

KeyArray[Platform::KEY_a] = Animation::TransformCommandType::TransformCommandYNegative;
KeyArray[Platform::KEY_A] = Animation::TransformCommandType::TransformCommandYNegative;

KeyArray[Platform::KEY_d] = Animation::TransformCommandType::TransformCommandYPositive;
KeyArray[Platform::KEY_D] = Animation::TransformCommandType::TransformCommandYPositive;
}

/// @brief Set a new mapping.
Expand Down Expand Up @@ -122,7 +133,13 @@ class OSRE_EXPORT TransformController : public Animation::AnimationControllerBas
void update(Animation::TransformCommandType cmdType) override;

private:
KeyboardMap mKeyboardMap;
struct TransformConfig {
f32 mScaleFactorPlus = 1.01f;
f32 mScaleFactorMinus = 0.99f;
f32 mRotateFactor = 0.01f;
f32 mTranslateFactor = 0.15f;
} mTransformConfig;
InputMap mInputMap;
RenderBackend::TransformMatrixBlock &mTransform;
};

Expand Down
1 change: 0 additions & 1 deletion src/Engine/RenderBackend/2D/CanvasRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ void dealloc(DrawCmd *cmd) {
delete cmd;
}


CanvasRenderer::CanvasRenderer(i32 numLayers, i32 x, i32 y, i32 w, i32 h) :
IRenderPath(),
mDirty(true),
Expand Down
Loading