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

EdvinLndh · 2024-09-25T13:16:11Z

Camera boundaries are set using constants for now, will probably look into setting the boundaries dynamically soon. #1682

…e camera movement.

heinezen

Looks good so far!

I have a few comments regarding the current status.

heinezen · 2024-09-29T12:08:09Z

libopenage/renderer/camera/camera.cpp

+	// 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;


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.

heinezen · 2024-09-29T12:10:51Z

libopenage/renderer/camera/camera.cpp

+                     std::optional<std::pair<float, float>> x_bounds,
+                     std::optional<std::pair<float, float>> z_bounds) {


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

heinezen · 2024-09-29T12:13:34Z

libopenage/renderer/camera/camera.cpp

+                      std::pair<float, float> x_bounds,
+                      std::pair<float, float> z_bounds,


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

heinezen · 2024-09-29T12:16:02Z

libopenage/renderer/camera/camera.h

@@ -15,6 +15,7 @@
 #include "renderer/camera/definitions.h"
 #include "renderer/camera/frustum_2d.h"
 #include "renderer/camera/frustum_3d.h"
+#include <optional>


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"

heinezen · 2024-09-29T12:18:12Z

libopenage/renderer/camera/camera.h

 	 * @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);


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

heinezen · 2024-09-29T12:31:47Z

libopenage/renderer/demo/demo_5.cpp

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

+	camera::CameraManager camera_manager(camera);


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.

heinezen · 2024-09-29T12:32:11Z

libopenage/renderer/demo/demo_6.cpp

@@ -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);


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

heinezen · 2024-09-29T12:35:46Z

libopenage/renderer/stages/camera/manager.cpp

+	x_bounds = std::make_pair(XMIN, XMAX);
+	z_bounds = std::make_pair(ZMIN, ZMAX);


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

heinezen · 2024-09-29T12:39:23Z

libopenage/renderer/stages/camera/manager.h

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


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.

heinezen · 2024-09-29T12:43:23Z

libopenage/renderer/stages/camera/manager.h

+	/**
+	 * 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;


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.

heinezen · 2024-10-18T03:08:27Z

@EdvinLndh Are you planning to continue this one soon-ish?

EdvinLndh · 2024-10-18T07:37:30Z

Sorry, I've been out of town for a few weeks, I'll look into it

heinezen · 2024-10-18T13:54:58Z

libopenage/renderer/camera/camera.h

+	 *              the positional vector.
+	 * @param camera_boundaries X and Z boundaries for the camera in the scene.
+	 */
+	void move_rel(Eigen::Vector3f direction, float delta, struct CameraBoundaries camera_boundaries);


Suggested change

void move_rel(Eigen::Vector3f direction, float delta, struct CameraBoundaries camera_boundaries);

void move_rel(Eigen::Vector3f direction, float delta, CameraBoundaries camera_boundaries);

struct is only necessary in C, but you can omit it in C++

heinezen · 2024-10-18T14:01:16Z

libopenage/renderer/camera/camera.h

+    CameraBoundaries(float x_min, float x_max, float z_min, float z_max)
+        : x_min(x_min), x_max(x_max), z_min(z_min), z_max(z_max) {}


For structs, we usually don't declare a constructor and use a braced initializer list to create objects, especially for trivial structs like this one. Thus, you don't need to define it here.

heinezen · 2024-10-18T14:05:40Z

libopenage/renderer/camera/camera.h

+ * Defines constant boundaries for the camera's view in the X and Z axes.
+ */
+struct CameraBoundaries {
+	const float x_min, x_max, z_min, z_max;


I'm not sure it makes sense to declare all members as const, since there's no reason why the camera boundaries should be immutable changed at all time. If we wanted a an immutable CameraBoundaries struct, we could just declare that as const instead with const CameraBoundaries example{..};

heinezen · 2024-10-20T14:50:34Z

I have converted this into a draft while you work on it. Once you are done, you can set it as ready with the "Ready for review" button above.

Renderer: Added camera boundaries to CameraManager and clamping to th…

70cd7c2

…e camera movement.

heinezen added area: renderer Concerns our graphics renderer improvement Enhancement of an existing component nice new thing ☺ A new feature that was not there before lang: c++ Done in C++ code labels Sep 25, 2024

heinezen linked an issue Sep 25, 2024 that may be closed by this pull request

Boundaries for camera movement #1682

Open

4 tasks

heinezen reviewed Sep 29, 2024

View reviewed changes

heinezen mentioned this pull request Oct 18, 2024

Boundaries for camera movement #1682

Open

4 tasks

EdvinLndh added 2 commits October 18, 2024 14:33

Refactoring and documentation.

4c08a94

Added constructor for CameraBoundaries.

12a795f

heinezen reviewed Oct 19, 2024

View reviewed changes

Struct refactoring

19169ad

heinezen marked this pull request as draft October 20, 2024 14:49

EdvinLndh marked this pull request as ready for review October 20, 2024 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EdvinLndh commented Sep 25, 2024 •

edited

Loading

heinezen left a comment

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen Sep 29, 2024

heinezen commented Oct 18, 2024

EdvinLndh commented Oct 18, 2024

heinezen Oct 18, 2024

heinezen Oct 18, 2024

heinezen Oct 18, 2024

heinezen commented Oct 20, 2024

		std::optional<std::pair<float, float>> x_bounds,
		std::optional<std::pair<float, float>> z_bounds) {

		std::pair<float, float> x_bounds,
		std::pair<float, float> z_bounds,

	// 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"

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

	void move_rel(Eigen::Vector3f direction, float delta, struct CameraBoundaries camera_boundaries);
	void move_rel(Eigen::Vector3f direction, float delta, CameraBoundaries camera_boundaries);

		CameraBoundaries(float x_min, float x_max, float z_min, float z_max)
		: x_min(x_min), x_max(x_max), z_min(z_min), z_max(z_max) {}

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

Are you sure you want to change the base?

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

Conversation

EdvinLndh commented Sep 25, 2024 • edited Loading

heinezen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heinezen commented Oct 18, 2024

EdvinLndh commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heinezen commented Oct 20, 2024

EdvinLndh commented Sep 25, 2024 •

edited

Loading