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

Jump to bottom

Renderer: Added camera boundaries to CameraManager and clamping #1682. #1691

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+103 −41
Open

Renderer: Added camera boundaries to CameraManager and clamping #1682. #1691

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
70cd7c2
Renderer: Added camera boundaries to CameraManager and clamping to th…
EdvinLndh Sep 25, 2024
4c08a94
Refactoring and documentation.
EdvinLndh Oct 18, 2024
12a795f
Added constructor for CameraBoundaries.
EdvinLndh Oct 18, 2024
19169ad
Struct refactoring
EdvinLndh Oct 20, 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
30 changes: 26 additions & 4 deletions libopenage/renderer/camera/camera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,37 @@ void Camera::look_at_coord(coord::scene3 coord_pos) {
this->look_at_scene(scene_pos);
}

void Camera::move_to(Eigen::Vector3f scene_pos) {
// TODO: Check and set bounds for where the camera can go and check them here
void Camera::move_to(Eigen::Vector3f scene_pos,
std::optional<std::pair<float, float>> x_bounds,
std::optional<std::pair<float, float>> z_bounds) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using two pairs for the bounds, it probably makes sense to make these parameters a struct and pass that.

// Calculate look at coordinates using same method as in look_at_scene(..)
auto y_delta = this->scene_pos[1] - scene_pos[1];
auto xz_distance = y_delta * std::numbers::sqrt3;

float side_length = xz_distance / std::numbers::sqrt2;
scene_pos[0] += side_length;
scene_pos[2] += side_length;
Copy link
Member

Choose a reason for hiding this comment

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

move_to should not do the look at calculations because it is intended to just move the camera to a position in the scene. Looking at a scene coordinate is already implemented in look_at_scene. This might break existing code where move_to is currently used.

The changes here might also break look_at_scene's behavior, since the method calls move_to but with the already computed camera position. Therefore, the look at calculations would be done a second time on the already translated camera position.


// Clamp the x coordinate if x_bounds are provided
if (x_bounds.has_value()) {
scene_pos[0] = std::clamp(scene_pos[0], x_bounds->first, x_bounds->second);
}

// Clamp the z coordinate if z_bounds are provided
if (z_bounds.has_value()) {
scene_pos[2] = std::clamp(scene_pos[2], z_bounds->first, z_bounds->second);
}

this->scene_pos = scene_pos;
this->moved = true;
}

void Camera::move_rel(Eigen::Vector3f direction, float delta) {
this->move_to(this->scene_pos + (direction * delta));
void Camera::move_rel(Eigen::Vector3f direction,
std::pair<float, float> x_bounds,
std::pair<float, float> z_bounds,
Copy link
Member

Choose a reason for hiding this comment

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

In move_to you made them optional, but here they are non-optional. Is there a reason for this?

float delta) {

this->move_to(this->scene_pos + (direction * delta), x_bounds, z_bounds);
}

void Camera::set_zoom(float zoom) {
Expand Down
9 changes: 7 additions & 2 deletions libopenage/renderer/camera/camera.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "renderer/camera/definitions.h"
#include "renderer/camera/frustum_2d.h"
#include "renderer/camera/frustum_3d.h"
#include <optional>
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick, but imports of standard libraries should be placed before local includes in our code. See our (arguably a bit hidden) code style examples:

openage/doc/code_style/mom.cpp

Lines 17 to 32 in 66098ce

// The associated header file comes first!
#include "mom.h"
// C++ std library includes follow, sorted alphabetically
#include <cmath>
#include <cstdio>
#include <iostream>
// External libraries are next, sorted alphabetically
#include <eigen3/Eigen/Dense>
#include <epoxy/gl.h>
// Local includes come last, sorted alphabetically
#include "../valve.h"
#include "half_life.h"
#include "log/log.h"



namespace openage::renderer {
Expand Down Expand Up @@ -84,7 +85,9 @@ class Camera {
*
* @param scene_pos New 3D position of the camera in the scene.
*/
void move_to(Eigen::Vector3f scene_pos);
void move_to(Eigen::Vector3f scene_pos, std::optional<std::pair<float, float>> x_bounds = std::nullopt,
std::optional<std::pair<float, float>> z_bounds = std::nullopt);
Copy link
Member

Choose a reason for hiding this comment

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

You should document arguments you add to methods in the docstrings above.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if you code formatted this but if you didn't try running clang-format over this file. The arguments should either be placed on the same line or in this case on a separate line each.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using std::optional and assigning std::nullopt as default value, you could also use regular non-optional floats and set the default value to std::numeric_limits::min()/std::numeric_limits::max(). Then the clamp operation will work the same, but you won't need to check has_value in the code.



/**
* Move the camera position in the direction of a given vector.
Expand All @@ -94,7 +97,9 @@ class Camera {
* value is multiplied with the directional vector before its applied to
* the positional vector.
*/
void move_rel(Eigen::Vector3f direction, float delta = 1.0f);
void move_rel(Eigen::Vector3f direction, std::pair<float, float> x_bounds,
std::pair<float, float> z_bounds,
float delta = 1.0f);
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above:

  • arguments should be documented in the doc string
  • formatting should be done for the arguments
  • you could assign default values here


/**
* Set the zoom level of the camera. Values smaller than 1.0f let the
Expand Down
10 changes: 6 additions & 4 deletions libopenage/renderer/demo/demo_5.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "renderer/shader_program.h"
#include "renderer/uniform_buffer.h"
#include "renderer/uniform_input.h"
#include "renderer/stages/camera/manager.h"


namespace openage::renderer::tests {
Expand All @@ -38,6 +39,7 @@ void renderer_demo_5(const util::Path &path) {
camera->resize(w, h);
});

camera::CameraManager camera_manager(camera);
Copy link
Member

Choose a reason for hiding this comment

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

The demos in this file should probably not use the camera manager because that introduces more unnecessary complexity to them. We try to show off specific features of the renderer in the code. As CameraManager is an interface between the input system and the renderer, it is technically an additional layer that does a bunch of updates which are unnecessary for the demo.

/* Display the subtextures using the meta information */
log::log(INFO << "Loading shaders...");

Expand Down Expand Up @@ -142,29 +144,29 @@ void renderer_demo_5(const util::Path &path) {

switch (key) {
case Qt::Key_W: { // forward
camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, -1.0f), 0.5f);
camera_manager.move_frame(camera::MoveDirection::FORWARD, 0.5f);
cam_update = true;

log::log(INFO << "Camera moved forward.");
} break;
case Qt::Key_A: { // left
// half the speed because the relationship between forward/back and
// left/right is 1:2 in our ortho projection.
camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, 1.0f), 0.25f);
camera_manager.move_frame(camera::MoveDirection::LEFT, 0.25f);
cam_update = true;

log::log(INFO << "Camera moved left.");
} break;
case Qt::Key_S: { // back
camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, 1.0f), 0.5f);
camera_manager.move_frame(camera::MoveDirection::BACKWARD, 0.5f);
cam_update = true;

log::log(INFO << "Camera moved back.");
} break;
case Qt::Key_D: { // right
// half the speed because the relationship between forward/back and
// left/right is 1:2 in our ortho projection.
camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, -1.0f), 0.25f);
camera_manager.move_frame(camera::MoveDirection::RIGHT, 0.25f);
cam_update = true;

log::log(INFO << "Camera moved right.");
Expand Down
10 changes: 6 additions & 4 deletions libopenage/renderer/demo/demo_6.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "renderer/shader_program.h"
#include "renderer/texture.h"
#include "renderer/uniform_buffer.h"
#include "renderer/stages/camera/manager.h"
#include "time/clock.h"
#include "util/path.h"
#include "util/vector.h"
Expand Down Expand Up @@ -52,16 +53,16 @@ void renderer_demo_6(const util::Path &path) {
// move_frame moves the camera in the specified direction in the next drawn frame
switch (key) {
case Qt::Key_W: { // forward
render_mgr.camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, -1.0f), 0.2f);
render_mgr.camera_manager->move_frame(camera::MoveDirection::FORWARD, 0.2f);
} break;
case Qt::Key_A: { // left
render_mgr.camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, 1.0f), 0.1f);
render_mgr.camera_manager->move_frame(camera::MoveDirection::LEFT, 0.1f);
} break;
case Qt::Key_S: { // back
render_mgr.camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, 1.0f), 0.2f);
render_mgr.camera_manager->move_frame(camera::MoveDirection::BACKWARD, 0.2f);
} break;
case Qt::Key_D: { // right
render_mgr.camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, -1.0f), 0.1f);
render_mgr.camera_manager->move_frame(camera::MoveDirection::RIGHT, 0.1f);
} break;
default:
break;
Expand Down Expand Up @@ -438,6 +439,7 @@ void RenderManagerDemo6::create_camera() {
1.0f / static_cast<float>(viewport_size[1])};
camera_unifs->update("inv_viewport_size", viewport_size_vec);
this->camera->get_uniform_buffer()->update_uniforms(camera_unifs);
this->camera_manager = std::make_shared<camera::CameraManager>(camera);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, using the camera manager is a bit overkill for this demo.

}

void RenderManagerDemo6::create_render_passes() {
Expand Down
4 changes: 4 additions & 0 deletions libopenage/renderer/demo/demo_6.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class Texture2d;

namespace camera {
class Camera;
class CameraManager;
}

namespace gui {
Expand Down Expand Up @@ -72,6 +73,9 @@ class RenderManagerDemo6 {
/// Camera
std::shared_ptr<camera::Camera> camera;

/// Camera manager
std::shared_ptr<camera::CameraManager> camera_manager;

/// Render passes
std::shared_ptr<renderer::RenderPass> obj_2d_pass;
std::shared_ptr<renderer::RenderPass> obj_3d_pass;
Expand Down
14 changes: 9 additions & 5 deletions libopenage/renderer/stages/camera/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "manager.h"

#include <eigen3/Eigen/Dense>
#include <numbers>

#include "renderer/camera/camera.h"
#include "renderer/uniform_buffer.h"
Expand All @@ -21,6 +22,9 @@ CameraManager::CameraManager(const std::shared_ptr<renderer::camera::Camera> &ca
camera->get_view_matrix(),
"proj",
camera->get_projection_matrix());

x_bounds = std::make_pair(XMIN, XMAX);
z_bounds = std::make_pair(ZMIN, ZMAX);
Copy link
Member

Choose a reason for hiding this comment

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

These members should be initialized in the initializer list in the constructor above.

}

void CameraManager::update() {
Expand All @@ -33,18 +37,18 @@ void CameraManager::move_frame(MoveDirection direction, float speed) {
case MoveDirection::LEFT:
// half the speed because the relationship between forward/back and
// left/right is 1:2 in our ortho projection.
this->camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, 1.0f), speed / 2);
this->camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, 1.0f), this->x_bounds, this->z_bounds, speed / 2);
break;
case MoveDirection::RIGHT:
// half the speed because the relationship between forward/back and
// left/right is 1:2 in our ortho projection.
this->camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, -1.0f), speed / 2);
this->camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, -1.0f), this->x_bounds, this->z_bounds, speed / 2);
break;
case MoveDirection::FORWARD:
this->camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, -1.0f), speed);
this->camera->move_rel(Eigen::Vector3f(-1.0f, 0.0f, -1.0f), this->x_bounds, this->z_bounds, speed);
break;
case MoveDirection::BACKWARD:
this->camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, 1.0f), speed);
this->camera->move_rel(Eigen::Vector3f(1.0f, 0.0f, 1.0f), this->x_bounds, this->z_bounds, speed);
break;

default:
Expand Down Expand Up @@ -83,7 +87,7 @@ void CameraManager::update_motion() {
move_dir += Eigen::Vector3f(1.0f, 0.0f, 1.0f);
}

this->camera->move_rel(move_dir, this->move_motion_speed);
this->camera->move_rel(move_dir, this->x_bounds, this->z_bounds, this->move_motion_speed);
}

if (this->zoom_motion_direction != static_cast<int>(ZoomDirection::NONE)) {
Expand Down
12 changes: 12 additions & 0 deletions libopenage/renderer/stages/camera/manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#pragma once

#include <memory>
#include <utility>


namespace openage::renderer {
Expand Down Expand Up @@ -143,6 +144,17 @@ class CameraManager {
* Uniform buffer input for the camera.
*/
std::shared_ptr<renderer::UniformBufferInput> uniforms;

/**
* x and z bounds for the camera.
*/
std::pair<float, float> x_bounds, z_bounds;
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick but the docstring should explain more about what these bounds are. Also, you should mention that the first value is the min bound and the second value is the max bound. However, if you follow up on the idea to make this a struct, it would probably not be necessary to document the latter.


/**
* Constant values for the camera bounds.
* TODO: Make boundaries dynamic based on map size.
*/
const float XMIN = 12.25f, XMAX = 32.25f, ZMIN = -8.25f, ZMAX = 12.25f;
Copy link
Member

Choose a reason for hiding this comment

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

You could move these to https://github.com/SFTtech/openage/blob/master/libopenage/renderer/camera/definitions.h but since they are meant only for testing, this doesn't really matter.

};

} // namespace camera
Expand Down
Loading