Allow shapes to be constructed with zero values

[juliohq]

Objective

Fixes #13332.

Solution

The assertion circumradius >= 0.0 to allow zero.

Are there any other shapes that need to be allowed to be constructed with zero?

[mamekoro]

Thanks for the pull request. I opened that issue, but it took me a while to investigate.

It should be sufficient to fix RegularPolygon.
I tested the following 2D and 3D shapes implementing Into<Mesh> and Primitive{2,3}d.

• 2D shapes
  • Annulus
  • Capsule2d
  • Circle
  • Ellipse
  • Rectangle
  • RegularPolygon
  • Triangle2d
• 3D shapes
  • Capsule3d
  • Cone
  • Cuboid
  • Cylinder
  • Plane3d
  • Sphere
  • Torus
  • Triangle3d

Only Circle and RegularPolygon cause a panic when initialized with zero values.
Circle meshes are instantiated using RegularPolygon (in CircleMeshBuilder), so fixing RegularPolygon should also fix Circle.

Edit:
Plane3d::new(normal: Vec3, half_size: Vec2) in v0.14.0-dev panics if the normal parameter is a zero vector. (half_size can be zero.)
This panic is acceptable to me since the normal parameter is a normal vector and it is important that its length is not zero.
(But I'm not an expert in 3DCG and would like to hear others' opinions.)

[juliohq]

@mamekoro I couldn't get Circle to panic. Could you share the test you've used?

[mamekoro]

I tested on the latest main branch.

cargo new circle
cd circle
git clone --depth=1 https://github.com/bevyengine/bevy
cargo add --path bevy

./src/main.rs is this;

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, startup)
        .run();
}

fn startup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(ColorMesh2dBundle {
        mesh: meshes.add(Circle::new(0.0)).into(),
        material: materials.add(Color::WHITE),
        ..default()
    });
}

The crash log is here;

thread '<unnamed>' panicked at /tmp/circle/bevy/crates/bevy_math/src/primitives/dim2.rs:729:9:
polygon has a non-positive radius
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `circle::startup`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

full test

It is difficult to tell which shape has panicked when testing multiple shapes at once, so toggle comments to select only one shape to test.

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, test2d)
        //.add_systems(Startup, test3d)
        .run();
}

fn test2d(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    commands.spawn(Camera2dBundle::default());

    // panics
    commands.spawn(ColorMesh2dBundle {
        mesh: meshes.add(Circle::new(0.0)).into(),
        material: materials.add(Color::WHITE),
        ..default()
    });

    // no panic
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes.add(Ellipse::new(0.0, 0.0)).into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});

    // no panic
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes.add(Annulus::new(0.0, 0.0)).into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});

    // no panic
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes
    //        .add(Triangle2d::new(Vec2::ZERO, Vec2::ZERO, Vec2::ZERO))
    //        .into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});

    // no panic
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes.add(Rectangle::new(0.0, 0.0)).into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});

    // panics
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes.add(RegularPolygon::new(0.0, 3)).into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});

    // no panic
    //commands.spawn(ColorMesh2dBundle {
    //    mesh: meshes.add(Capsule2d::new(0.0, 0.0)).into(),
    //    material: materials.add(Color::WHITE),
    //    ..default()
    //});
}

fn test3d(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    commands.spawn(Camera3dBundle {
        transform: Transform::from_xyz(0.0, 5.0, 5.0).looking_at(Vec3::ZERO, Vec3::Y),
        ..default()
    });
    commands.spawn(PointLightBundle {
        point_light: PointLight {
            shadows_enabled: true,
            intensity: 10_000_000.,
            range: 100.0,
            ..default()
        },
        transform: Transform::from_xyz(5.0, 10.0, 5.0),
        ..default()
    });

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Capsule3d::new(0.0, 0.0)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Cone {
    //        radius: 0.0,
    //        height: 0.0,
    //    }),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Cuboid {
    //        half_size: Vec3::ZERO,
    //    }),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Cylinder::new(0.0, 0.0)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic (Tested with a non-zero normal vector)
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Plane3d::new(Vec3::new(0.0, 1.0, 0.0), Vec2::ZERO)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Sphere::new(0.0)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Sphere::new(0.0).mesh().ico(4).unwrap()),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Sphere::new(0.0).mesh().uv(4, 4)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Torus::new(0.0, 0.0)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});

    // no panic
    //commands.spawn(PbrBundle {
    //    mesh: meshes.add(Triangle3d::new(Vec3::ZERO, Vec3::ZERO, Vec3::ZERO)),
    //    material: materials.add(StandardMaterial::default()).clone(),
    //    ..default()
    //});
}

[juliohq]

@mamekoro I just ran your test against my changes and they seem to solve the issue. Thank you.

[ramirezmike]

Looks good to me, although I agree with mockersf's suggestion

[mamekoro]

Should we reject -0.0?
assert!(circumradius.is_sign_positive()) panics when circumradius is -0.0, but assert!(circumradius >= 0.0) does not. Bevy v0.12 also doesn't panic on radius -0.0.

[alice-i-cecile]

Should we reject -0.0? assert!(circumradius.is_sign_positive()) panics when circumradius is -0.0, but assert!(circumradius >= 0.0) does not. Bevy v0.12 also doesn't panic on radius -0.0.

I would allow it: it should behave fine.