-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Frustum culling #1642
Conversation
Nice, I'll take a look over the weekend :) |
// 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. |
copying.md
Outdated
@@ -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... |
I'm gonna take this over to fix all problems and make it mergeable. |
how is the object size calculated currently? checking the current sprite sizes every frame? then we could do from the maximum of all its sprites instead during load? and the zooming we need anyway? :) |
It's done in the shader. The size is a combination of |
6522a7d
to
144485a
Compare
Okay the code should now work for 3D frustum culling. I will now add an optimized version for 2D sprites as @TheJJ suggested and a demo for showing off the functionality. |
67cf2d1
to
ae95823
Compare
Needed the current `time` for getting the correct position.
ae95823
to
eed0401
Compare
Almost everything should be ready feature-wise now. It's just documentation for the frustum usage & architecture that is missing. |
I disabled frustum culling for now, since it's not that performant right now. Currently, it just skips the uniform update, but checking the frustum is much more expensive than performing the uniform update by itself. To make the frustum culling worth our while, we should also exclude the renderables from the draw calls. This would require us to build the list of renderables every frame which can come with its own drawbacks. |
eed0401
to
8ba6a75
Compare
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.