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.

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

fix inspector rotation use XYZ rotate #482

Merged
merged 2 commits into from
Feb 29, 2024
Merged

fix inspector rotation use XYZ rotate #482

merged 2 commits into from
Feb 29, 2024

Conversation

VTui22
Copy link
Contributor

@VTui22 VTui22 commented Feb 28, 2024

No description provided.

@VTui22 VTui22 requested a review from T-rvw February 28, 2024 08:14
@@ -51,6 +53,7 @@ class TransformComponent final

#ifdef EDITOR_MODE
static bool m_doUseUniformScale;
cd::Vec3f m_inspectorEular = cd::Vec3f::Zero();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cd::Vec3f m_inspectorEular = cd::Vec3f::Zero();
cd::Vec3f m_rotateEular = cd::Vec3f::Zero();

As a transform component, it is better not to know what is Inspector because it is an editor concept.
What you need to know about transform component is that we need a temporary eular data storage.

Comment on lines 40 to 41
cd::Vec3f& GetInspectorEular() { return m_inspectorEular; }
void SetInspectorRotation(const cd::Vec3f& eular) { m_inspectorEular = eular; }
Copy link
Contributor

@T-rvw T-rvw Feb 28, 2024

Choose a reason for hiding this comment

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

Suggested change
cd::Vec3f& GetInspectorEular() { return m_inspectorEular; }
void SetInspectorRotation(const cd::Vec3f& eular) { m_inspectorEular = eular; }
cd::Vec3f& GetInspectorEular() { return m_inspectorEular; }
const cd::Vec3f& GetInspectorEular() const { return m_inspectorEular; }
void SetInspectorRotation(cd::Vec3f eular) { m_inspectorEular = cd::MoveTemp(eular); }

For data size larger than 8 bytes, it recommends to use move.

@@ -11,6 +11,36 @@
namespace ImGuiUtils
{

static cd::Matrix3x3 RotateX(float rad)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leverage template + if constexpr to write one function to implement same feature.

@@ -185,7 +215,7 @@ static bool ImGuiVectorProperty(const char* pName, T& value, cd::Unit unit = cd:
return dirty;
}

static bool ImGuiTransformProperty(const char* pName, cd::Transform& value)
static bool ImGuiTransformProperty(const char* pName, cd::Transform& value, cd::Vec3f& inspectorEular)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is just OK but doesn't look good in the call side. As a api user, it is strange to apply extra vec3f.

pitch = std::max(pitch, -89.9f);

value.SetRotation(cd::Quaternion::FromPitchYawRoll(pitch, eularAngles.y(), eularAngles.z()));
// XYZ Rotate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// XYZ Rotate

The comment is useless as you can get XYZ rotate information from codes in next line.

@T-rvw
Copy link
Contributor

T-rvw commented Feb 28, 2024

What you need to improve is to learn to stand in different positions to review your codes so that you can make your codes more generic.

  1. If I am in Engine project, is it suitable to use variable name such as InspectorRotation? Is there a more generic name?
  2. If I am other developers who firstly see your comments, is it useful for others to learn something?
  3. If I am editor developer who wants to use API such as ImGuiTransformProperty, is the interface easy to understand?

@T-rvw T-rvw merged commit 096981e into main Feb 29, 2024
2 checks passed
@T-rvw T-rvw deleted the fix_inspector_rotation branch February 29, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants