Allow opting-out of check_visibility system and creation of their own

[Phoqinu]

Allow users create to opt-out of bevy's check_visibility system and to create their own.

Personally I am not too happy with this. While it gets the job done it's quite hack-job. I would like to see a better pr in future which refactors how check_visibility and custom check visibility systems work and interact.

Also I think I need better comments for my additions.

Objective

• opt-out of bevy's check_visibility system
• properly (?) schedule your custom check_visibility system
• Fixes Custom entity visibility system. #17133

Solution

• Add marker component CustomVisibilityCheck and add Without<CustomVisibilityCheck> to check_visibility's query
• Add new variant to VisibilitySystems system set CustomCheckVisibility and order it after check_visibility

Testing

• Did you test these changes? If so, how?
  I tested it with sprites and mesh3d, meshmaterial3d entities

• Are there any parts that need more testing?
  I am not sure

• How can other people (reviewers) test your changes? Is there anything specific they need to know?
  They need to properly create the system and it's logic, add it to app and annotate entities with the marker component

• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
  Should be platform independent

---

Showcase

Users of this must do what check_visibility does except clearing camera's VisibleEntities or PreviousVisibleEntities.
You will have 2 responsibilities (not in order).

1. Set entity's ViewVisibility. You must either set it as visible or hidden. To do so you must use your custom method - After all you are opting-out because it's faster than parallel frustum culling using aabb checks and visibility ranges, right ? - while maybe respecting InheritedVisibility, NoFrustumCulling, VisibilityClass and others. It must be set every frame

2. Extend camera's VisibleEntities. If camera is active and entities are in it's view (see 1.) then you must add the entities to the correct visibility class inside VisibleEntities or they will be ignored. It must be set every frame

But other things like shadows depend on other components so if your system is wrong - if you will set wrong ViewVisibility/VisibleEntities - it will not change how shadows and etc are rendered.

Here's modified example `3d_shapes`

//! This example demonstrates the built-in 3d shapes in Bevy.
//! The scene includes a patterned texture and a rotation for visualizing the normals and UVs.
//!
//! You can toggle wireframes with the space bar except on wasm. Wasm does not support
//! `POLYGON_MODE_LINE` on the gpu.

use std::f32::consts::PI;

#[cfg(not(target_arch = "wasm32"))]
use bevy::pbr::wireframe::{WireframeConfig, WireframePlugin};
use bevy::{
    color::palettes::basic::SILVER,
    prelude::*,
    render::{
        render_asset::RenderAssetUsages,
        render_resource::{Extent3d, TextureDimension, TextureFormat},
    },
};

fn main() {
    App::new()
        .add_plugins((
            DefaultPlugins.set(ImagePlugin::default_nearest()),
            #[cfg(not(target_arch = "wasm32"))]
            WireframePlugin,
        ))
        .add_systems(Startup, setup)
        .add_systems(
            Update,
            (
                rotate,
                #[cfg(not(target_arch = "wasm32"))]
                toggle_wireframe,
            ),
        )
        // new
        .add_systems(PostUpdate, 
            // we must add it into proper schedule and set
            custom_visibility_check.in_set(bevy_render::view::VisibilitySystems::CustomCheckVisibility))
        .run();
}

/// A marker component for our shapes so we can query them separately from the ground plane
#[derive(Component)]
struct Shape;

const SHAPES_X_EXTENT: f32 = 14.0;
const EXTRUSION_X_EXTENT: f32 = 16.0;
const Z_EXTENT: f32 = 5.0;

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut images: ResMut<Assets<Image>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    let debug_material = materials.add(StandardMaterial {
        base_color_texture: Some(images.add(uv_debug_texture())),
        ..default()
    });

    let shapes = [
        meshes.add(Cuboid::default()),
        meshes.add(Tetrahedron::default()),
        meshes.add(Capsule3d::default()),
        meshes.add(Torus::default()),
        meshes.add(Cylinder::default()),
        meshes.add(Cone::default()),
        meshes.add(ConicalFrustum::default()),
        meshes.add(Sphere::default().mesh().ico(5).unwrap()),
        meshes.add(Sphere::default().mesh().uv(32, 18)),
    ];

    let extrusions = [
        meshes.add(Extrusion::new(Rectangle::default(), 1.)),
        meshes.add(Extrusion::new(Capsule2d::default(), 1.)),
        meshes.add(Extrusion::new(Annulus::default(), 1.)),
        meshes.add(Extrusion::new(Circle::default(), 1.)),
        meshes.add(Extrusion::new(Ellipse::default(), 1.)),
        meshes.add(Extrusion::new(RegularPolygon::default(), 1.)),
        meshes.add(Extrusion::new(Triangle2d::default(), 1.)),
    ];

    let num_shapes = shapes.len();

    for (i, shape) in shapes.into_iter().enumerate() {
        commands.spawn((
            Mesh3d(shape),
            MeshMaterial3d(debug_material.clone()),
            Transform::from_xyz(
                -SHAPES_X_EXTENT / 2. + i as f32 / (num_shapes - 1) as f32 * SHAPES_X_EXTENT,
                2.0,
                Z_EXTENT / 2.,
            )
            .with_rotation(Quat::from_rotation_x(-PI / 4.)),
            Shape,
            // new
            bevy_render::view::CustomVisibilityCheck // opt-out
        ));
    }

    let num_extrusions = extrusions.len();

    for (i, shape) in extrusions.into_iter().enumerate() {
        commands.spawn((
            Mesh3d(shape),
            MeshMaterial3d(debug_material.clone()),
            Transform::from_xyz(
                -EXTRUSION_X_EXTENT / 2.
                    + i as f32 / (num_extrusions - 1) as f32 * EXTRUSION_X_EXTENT,
                2.0,
                -Z_EXTENT / 2.,
            )
            .with_rotation(Quat::from_rotation_x(-PI / 4.)),
            Shape,
            // new
            bevy_render::view::CustomVisibilityCheck // opt-out
        ));
    }

    commands.spawn((
        PointLight {
            shadows_enabled: true,
            intensity: 10_000_000.,
            range: 100.0,
            shadow_depth_bias: 0.2,
            ..default()
        },
        Transform::from_xyz(8.0, 16.0, 8.0),
    ));

    // ground plane
    commands.spawn((
        Mesh3d(meshes.add(Plane3d::default().mesh().size(50.0, 50.0).subdivisions(10))),
        MeshMaterial3d(materials.add(Color::from(SILVER))),
    ));

    commands.spawn((
        Camera3d::default(),
        Transform::from_xyz(0.0, 7., 14.0).looking_at(Vec3::new(0., 1., 0.), Vec3::Y),
    ));

    #[cfg(not(target_arch = "wasm32"))]
    commands.spawn((
        Text::new("Press space to toggle wireframes"),
        Node {
            position_type: PositionType::Absolute,
            top: Val::Px(12.0),
            left: Val::Px(12.0),
            ..default()
        },
    ));
}

fn rotate(mut query: Query<&mut Transform, With<Shape>>, time: Res<Time>) {
    for mut transform in &mut query {
        transform.rotate_y(time.delta_secs() / 2.);
    }
}

/// Creates a colorful test pattern
fn uv_debug_texture() -> Image {
    const TEXTURE_SIZE: usize = 8;

    let mut palette: [u8; 32] = [
        255, 102, 159, 255, 255, 159, 102, 255, 236, 255, 102, 255, 121, 255, 102, 255, 102, 255,
        198, 255, 102, 198, 255, 255, 121, 102, 255, 255, 236, 102, 255, 255,
    ];

    let mut texture_data = [0; TEXTURE_SIZE * TEXTURE_SIZE * 4];
    for y in 0..TEXTURE_SIZE {
        let offset = TEXTURE_SIZE * y * 4;
        texture_data[offset..(offset + TEXTURE_SIZE * 4)].copy_from_slice(&palette);
        palette.rotate_right(4);
    }

    Image::new_fill(
        Extent3d {
            width: TEXTURE_SIZE as u32,
            height: TEXTURE_SIZE as u32,
            depth_or_array_layers: 1,
        },
        TextureDimension::D2,
        &texture_data,
        TextureFormat::Rgba8UnormSrgb,
        RenderAssetUsages::RENDER_WORLD,
    )
}

#[cfg(not(target_arch = "wasm32"))]
fn toggle_wireframe(
    mut wireframe_config: ResMut<WireframeConfig>,
    keyboard: Res<ButtonInput<KeyCode>>,
) {
    if keyboard.just_pressed(KeyCode::Space) {
        wireframe_config.global = !wireframe_config.global;
    }
}

// new
// custom check_visibility system
// this is the simplest impl which works for only 1 camera and doesn't do any visibility checking at all
fn custom_visibility_check(
    mut camera: Single<&mut bevy_render::view::VisibleEntities, With<Camera>>,
    mut shapes: Query<(Entity, &mut ViewVisibility, &bevy_render::view::VisibilityClass), (With<Shape>, With<bevy_render::view::CustomVisibilityCheck>)>,
) {
    for  (entity, mut view_visibility, visibility_class) in shapes.iter_mut() {
        if !**view_visibility {
            view_visibility.set();
        }

        for visibility_class_id in visibility_class.iter() {
            camera.get_mut(*visibility_class_id).push(entity);
        }
    }
}

Modified example highlights:

Adding our custom check_visibility system to App

        .add_systems(PostUpdate, 
            // we must add it into proper schedule and set
            custom_visibility_check.in_set(bevy_render::view::VisibilitySystems::CustomCheckVisibility))

Adding opt-out component to shape entities

        commands.spawn((
            Mesh3d(shape),
            MeshMaterial3d(debug_material.clone()),
            Transform::from_xyz(
                -SHAPES_X_EXTENT / 2. + i as f32 / (num_shapes - 1) as f32 * SHAPES_X_EXTENT,
                2.0,
                Z_EXTENT / 2.,
            )
            .with_rotation(Quat::from_rotation_x(-PI / 4.)),
            Shape,
            // new
            bevy_render::view::CustomVisibilityCheck // opt-out
        ));

and the system itself

// custom check_visibility system
// this is the simplest impl which works for only 1 camera and doesn't do any visibility checking at all
fn custom_visibility_check(
    mut camera: Single<&mut bevy_render::view::VisibleEntities, With<Camera>>,
    mut shapes: Query<(Entity, &mut ViewVisibility, &bevy_render::view::VisibilityClass), (With<Shape>, With<bevy_render::view::CustomVisibilityCheck>)>,
) {
    for  (entity, mut view_visibility, visibility_class) in shapes.iter_mut() {
        if !**view_visibility {
            view_visibility.set();
        }

        for visibility_class_id in visibility_class.iter() {
            camera.get_mut(*visibility_class_id).push(entity);
        }
    }
}

[alice-i-cecile]

This is messy, and I don't think we should merge this. IMO the right short-term fix is:

1. Disable the existing check_visibility system, probably with a run condition for now.
2. Add your own system that handles visibility checking the way that you like to CheckVisibility.

That provides a solid workaround (it would be better if we had system removing), and avoids cluttering the main codebase for now.

[Phoqinu]

Disable the existing check_visibility system, probably with a run condition for now.

My rationale for this was that I want to use the custom visibility checking just for some entities. That way anything else can be handled by bevy.

Eq: 2d game with tile world. Tiles would be handled by my custom visibility checking system while everything else - player and npc characters, dynamic structures on tiles, etc would be handled by bevy.

So disabling bevy's check_visibility system is undesirable,

[alice-i-cecile]

That much makes sense to me 🤔 This can be improved by removing the CustomCheckVisibility set, and adding a note to the system set about your marker component. Any new systems doing this should just be in CheckVisibility.

I'll try and get more opinions on this though.

[Phoqinu]

This can be improved by removing the CustomCheckVisibility set, and adding a note to the system set about your marker component. Any new systems doing this should just be in CheckVisibility.

This can be error prone because the custom system must be order after check_visibility system - if not then check_visibility will remove custom systems entities from camera and entities won't be rendered.

The example would be changed from this:

  add_systems(PostUpdate, 
            custom_visibility_check.in_set(VisibilitySystems::CustomCheckVisibility))

to this:

  add_systems(PostUpdate, 
            custom_visibility_check.in_set(VisibilitySystems::CheckVisibility).after(check_visibility))

I think this is from developer's UX worse because it's another thing you have to keep in mind.
But you will probably do it very very rarely so it would be ok ?

[alice-i-cecile]

This can be error prone because the custom system must be order after check_visibility system - if not then check_visibility will remove custom systems entities from camera and entities won't be rendered.

Ah, I see. In that case, I like your custom system set solution better than rolling it into CheckVisibility as a result.

[Phoqinu]

This new system set should be part of bevy ?

/// Label for systems which do their own custom visibility checking.
#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)]
pub struct CustomCheckVisibilitySystems;

[alice-i-cecile]

Yeah, your existing code is fine. I'm just not convinced that this is the best approach (broadly) to the problems outlined in the issue 🤔

[alice-i-cecile]

Swapping to a neutral review.

Mind has been changed.