[WIP] View Frustrum Culling using Octrees #1
Conversation
src/Magnum/Octree/CMakeLists.txt
Outdated
| # ${CMAKE_CURRENT_BINARY_DIR}/configure.h) | ||
|
|
||
| set(MagnumOctree_SRCS | ||
| Octree.hpp) |
There was a problem hiding this comment.
The library is empty (there are just templates which are not instantiated anywhere) and thus both MSVC and Clang/OSX build is failing. I'm not sure whether doing an instantiation.cpp file like in SceneGraph would make any sense, so I guess just not creating any library (and only installing/including the headers) will be enough.
Similarly for the DrawableGroup library below.
Oh, and I guess the *.hpp file has to be included by the users, so it should be in MagnumOctree_HEADERS I think?
ACapo
force-pushed
the
octree
branch
3 times, most recently
from
e51be56
to
2beaaaf
Compare
Squareys
force-pushed
the
octree
branch
2 times, most recently
from
2f8c1c5
to
e08dd78
Compare
|
So, here's an overview of what is left TODO:
That's still quite alot, but maybe, if we skip a few things (like allocation optimization or QuadTree) for now and mark this as experimental API, we can get this ancient PR finally merged. |
src/Magnum/Octree/CMakeLists.txt
Outdated
|
|
||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CORRADE_CXX_FLAGS}") | ||
|
|
||
| include_directories(${MAGNUM_INCLUDE_DIRS} ${CMAKE_CURRENT_BINARY_DIR}) |
There was a problem hiding this comment.
None of the above should be needed anymore.
src/Magnum/Octree/CMakeLists.txt
Outdated
| target_include_directories(MagnumOctree PUBLIC | ||
| ${PROJECT_SOURCE_DIR}/src | ||
| ${PROJECT_BINARY_DIR}/src | ||
| $<TARGET_PROPERTY:Magnum::Magnum,INTERFACE_INCLUDE_DIRECTORIES>) |
There was a problem hiding this comment.
The Magnum include directories were already added by the target_link_libraries() call above.
There was a problem hiding this comment.
Yeah, that's due to moving from the old CMake files to the new system, seems I forgot to clean up quite alot.
src/Magnum/Octree/CMakeLists.txt
Outdated
| if(BUILD_TESTS) | ||
| add_subdirectory(Test) | ||
| endif() | ||
|
|
src/Magnum/Octree/Octree.h
Outdated
| /** @file | ||
| * @brief Class @ref Magnum::Octree::Octree | ||
| * | ||
| * @author Andrea Capobianco |
There was a problem hiding this comment.
Please don't use the @author command. Author names will be mentioned in the CREDITS.md file.
src/Magnum/Octree/Octree.h
Outdated
| #include <Magnum/Shapes/AxisAlignedBox.h> | ||
|
|
||
| #include <vector> | ||
| #include "visibility.h" |
There was a problem hiding this comment.
Please use absolute includes, i.e. Magnum/Octree/visibility.h.
| const Vector4 bottom{mvp.row(3) + mvp.row(1)}; | ||
| const Vector4 top{mvp.row(3) - mvp.row(1)}; | ||
| const Vector4 near{mvp.row(3) + mvp.row(2)}; | ||
| const Vector4 far{mvp.row(3) - mvp.row(2)}; |
There was a problem hiding this comment.
Wasn't there a function for this somewhere? If not, can there be such a function? Or the Frustum class we were talking about?
| # DEALINGS IN THE SOFTWARE. | ||
| # | ||
|
|
||
| corrade_add_test(OctreeDrawableGroupTest OctreeDrawableGroupTest.cpp LIBRARIES MagnumOctreeDrawableGroup ${MAGNUM_LIBRARY} ${MAGNUM_SCENEGRAPH_LIBRARIES}) |
There was a problem hiding this comment.
Again, don't use old-style CMake. Both of these libraries should be linked by the MagnumOctreeDrawableGroup library anyway so no need to specify them implicitly.
| Copyright © 2010, 2011, 2012, 2013, 2014, 2015 | ||
| Vladimír Vondruš <[email protected]> | ||
| Copyright © 2015 | ||
| Andrea Capobianco <[email protected]> |
| #include "Magnum/SceneGraph/OctreeDrawableGroup.hpp" | ||
|
|
||
| using namespace std; | ||
| using namespace Corrade; |
| LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
| FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
| DEALINGS IN THE SOFTWARE. | ||
| */ |
There was a problem hiding this comment.
This header doesn't look like it should, making automated updates very hard to do.
There was a problem hiding this comment.
Whatever went wrong there 😅
✔️
modules/FindMagnumExtras.cmake
Outdated
| @@ -12,7 +12,7 @@ | |||
| # | |||
| # This command alone is useless without specifying the components: | |||
| # | |||
| # (none yet) | |||
| # Octree - Octree BSP structure for Magnum::SceneGraph | |||
There was a problem hiding this comment.
I would just say "Octree library" here and align the - ... at the same column as the above list
modules/FindMagnumExtras.cmake
Outdated
| @@ -65,6 +66,7 @@ | |||
|
|
|||
| # Corrade library dependencies | |||
| set(_MAGNUMEXTRAS_CORRADE_DEPENDENCIES ) | |||
|
|
|||
modules/FindMagnumExtras.cmake
Outdated
|
|
||
| if(_component MATCHES SceneGraph) | ||
| set(_MAGNUMEXTRAS_${_COMPONENT}_MAGNUM_DEPENDENCIES SceneGraph) | ||
| endif() |
There was a problem hiding this comment.
I'm not sure how the SceneGraph component of extras will interact with SceneGraph component of Magnum, so please just merge all that into the Octree library as I said in the previous review.
modules/FindMagnumExtras.cmake
Outdated
|
|
||
| # Mark the dependencies as required if the component is also required | ||
| if(MagnumExtras_FIND_REQUIRED_${_component}) | ||
| foreach(_dependency ${_MAGNUMEXTRAS_${_COMPONENT}_DEPENDENCIES}) | ||
| foreach(_dependency ${}) |
modules/FindMagnumExtras.cmake
Outdated
| @@ -117,7 +128,7 @@ endif() | |||
|
|
|||
| # Component distinction (listing them explicitly to avoid mistakes with finding | |||
| # components from other repositories) | |||
| set(_MAGNUMEXTRAS_LIBRARY_COMPONENTS "^$") | |||
| set(_MAGNUMEXTRAS_LIBRARY_COMPONENTS "(Octree|SceneGraph)") | |||
Codecov Report
@@ Coverage Diff @@
## master #1 +/- ##
==========================================
+ Coverage 19.24% 27.13% +7.89%
==========================================
Files 35 39 +4
Lines 1408 1566 +158
==========================================
+ Hits 271 425 +154
- Misses 1137 1141 +4
Continue to review full report at Codecov.
|
Squareys
force-pushed
the
octree
branch
6 times, most recently
from
fc7855a
to
f6ad80b
Compare
| int corners = 0; | ||
|
|
||
| for(UnsignedByte c = 0; c < 8; ++c) { | ||
| const Math::Vector3<T> corner = Math::lerp(min, max, Math::BoolVector<3>{c}); |
There was a problem hiding this comment.
If doing intersection test between an AABB and a Frustum, only 2 vertices is needed. This is an optimization you can consider.
For technical details, you can read the chapter, Geometric Approach - Testing Boxes II in this article.
There was a problem hiding this comment.
Or you can directly use aabbFrustum from Magnum\Math\Intersection.h
There was a problem hiding this comment.
Hi @bigbike!
Currently neither @ACapo nor I am actively working on this PR, especially since we figured out that using aabbFrustum (which I contributed btw :) mosra/magnum@04ca48c) directly was just as fast for our use case (we only had around 60-80 objects) and building the octree actually caused more overhead...
This is still in the back of my mind and I really want to finish it, but at the moment I can't take the time.
Best, Jonathan.
There was a problem hiding this comment.
Hi @Squareys ,
Good to see your reply!
Before commenting yesterday, I read the source code of aabbFrustum, which was implemented in a very elegant, efficient manner. A pieces of very high-quality code. Did not know the author was you. I like it a lot. :-)
I like the code in this PR as well. Do not give up! Such functionality is rather necessary and important especially when rendering large-scale, static, geometrically-complex models.
Nice meeting you in this PR. Thanks!
Hello @mosra,
this is what I've been working on together with @Squareys for the last few months. It's in a working state but there's still some cleanup work to do. I'd appreciate if you could take a look at the code and point out major issues you find.
Greetings,
@ACapo