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
Frustum culling #1642
base: master
Are you sure you want to change the base?
Frustum culling #1642
Conversation
Fixed merge conflict
Fixed styling
Fixed various checks
Nice, I'll take a look over the weekend :) |
this->look_at_scene(Eigen::Vector3f(0.0f, 0.0f, 0.0f)); | ||
|
||
resources::UBOInput view_input{"view", resources::ubo_input_t::M4F32}; | ||
resources::UBOInput proj_input{"proj", resources::ubo_input_t::M4F32}; | ||
auto ubo_info = resources::UniformBufferInfo{resources::ubo_layout_t::STD140, {view_input, proj_input}}; | ||
this->uniform_buffer = renderer->add_uniform_buffer(ubo_info); | ||
|
||
// Make the frustum slightly bigger than the camera's view to ensure objects on the boundary get rendered | ||
float real_zoom = 0.7f * this->default_zoom_ratio * this->zoom; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The factor used here (0.7f
) should be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you don't need to calculate the frustum here, since line 30 moves the camera (and calls move_to
)
static const float near_distance = 0.01f; | ||
static const float far_distance = 100.0f; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be configured for the frustum class and not in the camera file.
float real_zoom = 0.7f * this->default_zoom_ratio * this->zoom; | ||
frustum.Recalculate(this->viewport_size, near_distance, far_distance, this->scene_pos, cam_direction, Eigen::Vector3f(0.0f, 1.0f, 0.0f), real_zoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for the factor.
float real_zoom = 0.7f * this->default_zoom_ratio * this->zoom; | ||
frustum.Recalculate(viewport_size, near_distance, far_distance, scene_pos, cam_direction, Eigen::Vector3f(0.0f, 1.0f, 0.0f), real_zoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for the factor.
/** | ||
* Is frustum culling enabled? If true, perform frustum culling. | ||
* If false, all frustum checks will return true | ||
*/ | ||
bool frustum_culling; | ||
|
||
/** | ||
* The frustum used to cull objects | ||
* Will be recalculated regardless of whether frustum culling is enabled | ||
*/ | ||
Frustum frustum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this for a while and I think it's better if the frustum is not a camera member for now. Instead, a frustum should be created and returned by the camera using a get_frustum(..)
function. The reasoning behind this is that frustum culling will likely be more configurable if the Frustum
is directly exposed. Accessing it through the camera class can make it harder to activate/deactivate when debugging. This means more frustums will be created at first (once per frame), but I think this is the better short-term option right now.
To create the frustum, you can add a function like this:
/**
* Get a frustum for this camera
*
* @param scalefactor Resize the frustum to ensure objects on the boundary get rendered
*/
Frustum get_frustum(float scalefactor)
scalefactor
would be the variabe that should be configurable that I mentioned earlier (the 0.7f
one).
// Skip empty lines and comments | ||
if (line.empty() || line.substr(0, 1) == "#") { | ||
// Skip empty lines, lines with carriage returns, and comments | ||
if (line.empty() || line.substr(0, 1) == "#" || line[0] == '\r') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, these should be removed by the string splitter
// Skip empty lines and comments | ||
if (line.empty() || line.substr(0, 1) == "#") { | ||
// Skip empty lines, lines with carriage returns, and comments | ||
if (line.empty() || line.substr(0, 1) == "#" || line[0] == '\r') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
@@ -56,6 +56,11 @@ std::string parse_imagefile(const std::vector<std::string> &args) { | |||
// it should result in an error if wrongly used here. | |||
|
|||
// Call substr() to get rid of the quotes | |||
// If the line ends in a carriage return, remove it as well | |||
if (args[1][args[1].size() - 1] == '\r') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
// Skip empty lines and comments | ||
if (line.empty() || line.substr(0, 1) == "#") { | ||
// Skip empty lines, lines with carriage returns, and comments | ||
if (line.empty() || line.substr(0, 1) == "#" || line[0] == '\r') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
@@ -58,6 +58,10 @@ void WorldRenderStage::update() { | |||
std::unique_lock lock{this->mutex}; | |||
auto current_time = this->clock->get_real_time(); | |||
for (auto &obj : this->render_objects) { | |||
if (!obj->within_camera_frustum(this->camera)) { | |||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still render the object btw, it just skips the update :D
I'll have to fix this in a separate PR as it is not possile right now to remove specific objects from the render pass.
I only found a few minor issues with this and a have restructering reques. It already looks quite nice! You also have to rebase and resolve the merge conflicts. |
@@ -153,6 +153,7 @@ _the openage authors_ are: | |||
| Astitva Kamble | askastitva | astitvakamble5 à gmail dawt com | | |||
| Haoyang Bi | AyiStar | ayistar à outlook dawt com | | |||
| Michael Seibt | RoboSchmied | github à roboschmie dawt de | | |||
| Nikhil Ghosh | NikhilGhosh75 | nghosh606 à gmail dawt com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CI fails because you are missing an |
at the end of the table. Then the mailmap entry is also not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we calculate a containing circle for each object on creation and when resizing it? then we only have to check one coordinate/distance for each object to cull it.
I think that would be a bit more complicated since we would have to calculate it for every animation sprite and not just per object. And then we also have to apply zoom scaling... |
Implemented Frustum Culling into the renderer. Currently only applies to the world renderer. The camera has a frustum data structure to allow it to cull objects outside the frustum.
An additional stresstest has been created for the renderer to test out frustum culling.
Additional fixes to loading files on windows that have the special \r character are also included in this pull request.