Skip to content
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

[Merged by Bors] - add a SpatialBundle with visibility and transform components #5344

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions crates/bevy_gltf/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@ use bevy_render::{
skinning::{SkinnedMesh, SkinnedMeshInverseBindposes},
Indices, Mesh, VertexAttributeValues,
},
prelude::BaseBundle,
primitives::{Aabb, Frustum},
render_resource::{AddressMode, Face, FilterMode, PrimitiveTopology, SamplerDescriptor},
renderer::RenderDevice,
texture::{CompressedImageFormats, Image, ImageSampler, ImageType, TextureError},
view::{VisibilityBundle, VisibleEntities},
view::VisibleEntities,
};
use bevy_scene::Scene;
#[cfg(not(target_arch = "wasm32"))]
use bevy_tasks::IoTaskPool;
use bevy_transform::{components::Transform, TransformBundle};
use bevy_transform::components::Transform;

use bevy_utils::{HashMap, HashSet};
use gltf::{
Expand Down Expand Up @@ -465,8 +466,7 @@ async fn load_gltf<'a, 'b>(

world
.spawn()
.insert_bundle(TransformBundle::identity())
.insert_bundle(VisibilityBundle::default())
.insert_bundle(BaseBundle::visible_identity())
.with_children(|parent| {
for node in scene.nodes() {
let result = load_node(
Expand Down Expand Up @@ -705,10 +705,9 @@ fn load_node(
) -> Result<(), GltfError> {
let transform = gltf_node.transform();
let mut gltf_error = None;
let mut node = world_builder.spawn_bundle(TransformBundle::from(Transform::from_matrix(
let mut node = world_builder.spawn_bundle(BaseBundle::from(Transform::from_matrix(
Mat4::from_cols_array_2d(&transform.matrix()),
)));
node.insert_bundle(VisibilityBundle::default());

node.insert(node_name(gltf_node));

Expand Down
59 changes: 59 additions & 0 deletions crates/bevy_render/src/base_bundle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
use bevy_ecs::prelude::Bundle;
use bevy_transform::prelude::{GlobalTransform, Transform};

use crate::view::{ComputedVisibility, Visibility};

/// A [`Bundle`] with the following [`Component`](bevy_ecs::component::Component)s:
/// * [`Visibility`] and [`ComputedVisibility`], which describe the visibility of an entity
/// * [`Transform`] and [`GlobalTransform`], which describe the position of an entity
///
/// * To show or hide an entity, you should set its [`Visibility`].
/// * To get the computed visibility of an entity, you should get its [`ComputedVisibility`].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe note that it is only valid after the visibility systems have been run? (check_visibility and visibility_propagate, I think.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the one hand, this is useful information. On the other hand, idk if we should be duplicating it across every bundle that includes ComputedVisibility. ComputedVisibility does have this "label dependency" information encoded in the relevant fields already. But it is probably worth also including that in the main ComputedVisibility docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not something I think we should block on here though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// * To place or move an entity, you should set its [`Transform`].
/// * To get the global position of an entity, you should get its [`GlobalTransform`].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just global position, but global transform. :) Maybe note that it is only valid for the current frame after transform propagation has been run?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the one hand, this is useful information. On the other hand, idk if we should be duplicating it across every bundle that includes GlobalTransform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree that "global transform" is preferable to "global position".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// * For hierarchies to work correctly, you must have all four components.
/// * You may use the [`BaseBundle`] to guarantee this.
mockersf marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Bundle, Debug, Default)]
pub struct BaseBundle {
mockersf marked this conversation as resolved.
Show resolved Hide resolved
/// The visibility of the entity.
pub visibility: Visibility,
/// The computed visibility of the entity.
pub computed: ComputedVisibility,
/// The transform of the entity.
pub transform: Transform,
/// The global transform of the entity.
pub global_transform: GlobalTransform,
}

impl BaseBundle {
/// Creates a new [`BaseBundle`] from a [`Transform`].
///
/// This initializes [`GlobalTransform`] as identity, and visibility as visible
#[inline]
pub const fn from_transform(transform: Transform) -> Self {
BaseBundle {
local: transform,
// Note: `..Default::default()` cannot be used here, because it isn't const
..Self::visible_identity()
}
}

/// Creates a new identity [`BaseBundle`], with no translation, rotation, and a scale of 1
/// on all axes.
#[inline]
pub const fn visible_identity() -> Self {
mockersf marked this conversation as resolved.
Show resolved Hide resolved
BaseBundle {
local: Transform::identity(),
global: GlobalTransform::identity(),
visibility: Visibility::visible(),
computed: ComputedVisibility::visible(),
}
}
}

impl From<Transform> for BaseBundle {
#[inline]
fn from(transform: Transform) -> Self {
Self::from_transform(transform)
}
}
2 changes: 2 additions & 0 deletions crates/bevy_render/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
extern crate core;

mod base_bundle;
pub mod camera;
pub mod color;
pub mod extract_component;
Expand All @@ -22,6 +23,7 @@ pub use extract_param::Extract;
pub mod prelude {
#[doc(hidden)]
pub use crate::{
base_bundle::BaseBundle,
camera::{Camera, OrthographicProjection, PerspectiveProjection},
color::Color,
mesh::{shape, Mesh},
Expand Down
23 changes: 22 additions & 1 deletion crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,40 @@ pub struct Visibility {

impl Default for Visibility {
fn default() -> Self {
Self::visible()
}
}

impl Visibility {
/// Creates a new [`Visibility`], set as visible
pub const fn visible() -> Self {
mockersf marked this conversation as resolved.
Show resolved Hide resolved
Self { is_visible: true }
}
}

/// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering
#[derive(Component, Clone, Reflect, Debug, Eq, PartialEq, Default)]
#[derive(Component, Clone, Reflect, Debug, Eq, PartialEq)]
#[reflect(Component)]
pub struct ComputedVisibility {
is_visible_in_hierarchy: bool,
is_visible_in_view: bool,
}

impl Default for ComputedVisibility {
fn default() -> Self {
Self::visible()
mockersf marked this conversation as resolved.
Show resolved Hide resolved
}
}

impl ComputedVisibility {
/// Creates a new [`ComputedVisibility`], set as visible
pub const fn visible() -> Self {
Self {
is_visible_in_hierarchy: true,
is_visible_in_view: true,
}
}

/// Whether this entity is visible to something this frame. This is true if and only if [`Self::is_visible_in_hierarchy`] and [`Self::is_visible_in_view`]
/// are true. This is the canonical method to call to determine if an entity should be drawn.
/// This value is updated in [`CoreStage::PostUpdate`] during the [`VisibilitySystems::CheckVisibility`] system label. Reading it from the
Expand Down
3 changes: 1 addition & 2 deletions examples/animation/animated_transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,7 @@ fn setup(
.insert_bundle((planet, player))
.with_children(|p| {
// This entity is just used for animation, but doesn't display anything
p.spawn_bundle(TransformBundle::default())
.insert_bundle(VisibilityBundle::default())
p.spawn_bundle(BaseBundle::default())
// Add the Name component
.insert(orbit_controller)
.with_children(|p| {
Expand Down