Refactor camera code #474
Refactor camera code #474
Conversation
IAmNotHanni
requested review from
westernheld,
movabo,
James-Allenby,
yeetari and
IceflowRE
| @@ -10,71 +10,71 @@ | |||
| namespace inexor::vulkan_renderer { | |||
|
|
|||
| namespace directions { | |||
| /// The default value of the camera's front vector. | |||
There was a problem hiding this comment.
Do we use or not the ending period?
There was a problem hiding this comment.
OK found it, we use periods at the end of the sentences :D
Already "discussed" on Discord.
| @@ -10,71 +10,71 @@ | |||
| namespace inexor::vulkan_renderer { | |||
|
|
|||
| namespace directions { | |||
| /// The default value of the camera's front vector. | |||
There was a problem hiding this comment.
OK found it, we use periods at the end of the sentences :D
Already "discussed" on Discord.
| /// @note We will implement more camera types in the future | ||
| /// @param type The camera type |
There was a problem hiding this comment.
| /// @note We will implement more camera types in the future | |
| /// @param type The camera type | |
| /// @param type The camera type | |
| /// @note We will implement more camera types in the future |
Param before notes.
| constexpr glm::vec3 DEFAULT_RIGHT{0.0f, 1.0f, 0.0f}; | ||
| /// The default value of the camera's up vector. | ||
| /// The default value of the camera's up vector | ||
| constexpr glm::vec3 DEFAULT_UP{0.0f, 0.0f, 1.0f}; | ||
| } // namespace directions | ||
|
|
||
| enum class CameraMovement { FORWARD, BACKWARD, LEFT, RIGHT }; |
There was a problem hiding this comment.
Iam not a friend of using an enum for indicating a camera movement. I would allow direct manipulations via vectors.
In general i don't really like the current state of this camera class.
This camera class is already highly specified and implemented and thus limited in adding new camera types.
By handling inputs and the resulting camera movement.
I would suggest to make a more general camera class with the most important methods like direction, position, plane, etc.
Then inherit from this base camera and implement a PlayerCamera which has more traits like rotation speed, movement speed etc.
In my eyes is camera movement already part of the game (user) code and not the engine code. We should only provide callbacks or signals to inputs.
There was a problem hiding this comment.
Nice work, though I'm not a huge fan of using ThisType &foo() { return *this; } everywhere (imo it's main use case is for builders). Also I agree with Iceflower that the camera code would be nicer if it ws inheritance based, something like:
struct Camera {
virtual void update();
virtual glm::vec3f forward() const;
virtual glm::mat4f view_matrix() const;
};
class FreeCamera : public Camera {};
|
This has lower priority atm. I will work on other pull requests for now :) |
Closes #411
Blocked by #518
Overview