-
-
Notifications
You must be signed in to change notification settings - Fork 319
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
Should boundingbox(scene) also check child scenes? #4135
Comments
In my opinion For example Axis and LScene have a blockscene in pixel space and a child scene in Axis space. It makes no sense for that child scene to consider pixel bounding boxes in its own limits. It also makes no sense to inherit transformations here, since they apply to different things. That should be removed too, imo, and that tutorial/example should be reworked to use just one scene instead. For example you could use this instead: function robotarm!(s)
cam3d!(s; lookat=Vec3f(4, 0, 0), eyeposition=Vec3f(0, 8, 0))
p1 = mesh!(s, Rect3f(Vec3f(0, -0.1, -0.1), Vec3f(5, 0.2, 0.2)))
p2 = mesh!(s, Rect3f(Vec3f(0, -0.1, -0.1), Vec3f(5, 0.2, 0.2)), color=:red, transformation = Transformation(p1.transformation))
translate!(p2, 5, 0, 0)
p3 = mesh!(s, Rect3f(Vec3f(-0.2), Vec3f(0.4)), color=:blue, transformation = Transformation(p2.transformation))
translate!(p3, 5, 0, 0)
end
parent = Scene()
robotarm!(parent) # looks good
center!(parent) # also good
``` |
Thank you for the response, it's helpful in forming a better mental model of Scene/Transformation graphs. The tutorial does also show the lego animation using just the Transformation graph, similar to what you've suggested. Though it wasn't obvious to me that the other style isn't recommended. It initially seemed convenient and consistent with the docs to use scenes with a shared camera as just "collections of plots+scenes" in some hierarchy to avoid managing scene and transformation hierarchies separately. It's clear to me now that always including child scenes in boundingbox is just wrong (e.g. boundingbox(lscene.blockscene) should not add the bounding box of lscene.scene). Maybe it could make sense to do if the child shares the same camera and is in the same space if the style used in the tutorial is to be supported. Though I think the idea you're getting at is that we just happen to have a default argument like |
Currently bounding_box loops over the plots in a scene, but not the plots in child scenes. Is that the intended behavior?
This leads to some confusing behavior of
center!
. The issue is especially confusing for e.g. a parent scene with children, but no direct plots in the parent scene.MWE adapting a portion of the scene graph tutorial:
Some hints from repl output:
This also means that if
parent
is from an LScene in a Figure one must usesave(filename, fig; update=false)
ascenter!
will be called on the lscene.scene. I first hit this issue while trying to save without setting update=false and was wondering why my output files were blank.Maybe this case should be considered for anyone working on #2881?
Though maybe a simple fix would be for
foreach_plot(f, s::Scene)
to recursively apply f to all plots in child scenes (maybe as a kwarg option). I could draft a PR if that's welcome.The text was updated successfully, but these errors were encountered: