Dynamic culling#1095
Conversation
facebook-github-bot
added
the
CLA Signed
|
|
||
| class SceneNode : public MagnumObject { | ||
| class SceneNode : public MagnumObject, | ||
| public Magnum::SceneGraph::AbstractFeature3D { |
There was a problem hiding this comment.
My biggest concern comes from this inheritance. SceneNode is just a pure scene graph node, inherited from the MagnumObject, and a "feature" is an attachment on that node.
Logically, a class should not be a node and an attachment to that node at the same time. This is not a good design.
Can we do it using "composition method" instead of "inheritance method"? Namely you created a new feature inherited from AbstractFeature3D, and add a "feature pointer" to this new feature in the SceneNode class?
There was a problem hiding this comment.
This is the way it is recommended by the magnum docs and I believe it is the only way to do it: https://doc.magnum.graphics/magnum/scenegraph.html#scenegraph-features-caching
There was a problem hiding this comment.
Logically, a class should not be a node and an attachment to that node at the same time.
That's supported by design, and perfectly fine. (Unless your coding guidelines ban multiple inheritance.)
Could be also a member variable, but then everything becomes more verbose, and you'll need another derived class and...
| else { | ||
| if (!worldCumulativeBB_) | ||
| worldCumulativeBB_ = { | ||
| geo::getTransformedBB(getCumulativeBB(), absoluteTransformation_)}; |
There was a problem hiding this comment.
Glad to see geo::getTransformedBB becomes useful in this performance-critical project. I remembered I optimized this function so that it became 13x faster.
There was a problem hiding this comment.
Yeah! The existence of that function and it being very fast is what makes this work well :)
|
@mosra: would you take a pass? Thanks! |
| node.setFrustumPlaneIndex(*culledPlane); | ||
| } | ||
| // if it has value, it means the aabb is culled | ||
| return (culledPlane != Cr::Containers::NullOpt); |
There was a problem hiding this comment.
| return (culledPlane != Cr::Containers::NullOpt); | |
| return !!culledPlane; |
works too
There was a problem hiding this comment.
Yeah, but I do like the explicitness of this
| if (isDirty()) | ||
| return absoluteTransformation().translation(); | ||
| else | ||
| return absoluteTransformation_.translation(); |
There was a problem hiding this comment.
I think this should instead be
| if (isDirty()) | |
| return absoluteTransformation().translation(); | |
| else | |
| return absoluteTransformation_.translation(); | |
| return setClean(), absoluteTransformation_.translation(); |
(or with less code golf, possibly). Basically you're going through the pain of calculating the absolute transformation but then you throw it away so next time absoluteTransformation() gets called, it will calculate it from scratch again, instead of reusing the cached value.
Before using the cached value explicitly request object cleaning by calling
object()->setClean().
There was a problem hiding this comment.
The rationale here was that this method is marked as const (and makes sense to be const). I can cast around that tho
There was a problem hiding this comment.
ooh, right
(add a non-const overload that saves the work at least in 50% of cases? 😄)
There was a problem hiding this comment.
Yeah, that's a good idea.
6 participants
Motivation and Context
Culling for dynamic objects. This updates the local AABB to be in world coordinates if a scene node doesn't have a precomputed AABB. Uses caching and lazy computation to be efficient (@mosra I think I have done this correctly, but I would appreciate a double check).
How Has This Been Tested
Tests pass, in the viewer now you can see the number of culled objects increase as you add objects.
Types of changes
New feature (non-breaking change which adds functionality)