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

Add Mesh transformation #11454

Merged
merged 11 commits into from
Jan 29, 2024
Merged

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jan 21, 2024

Objective

It can sometimes be useful to transform actual Mesh data without needing to change the Transform of an entity. For example, one might want to spawn a circle mesh facing up instead of facing Z, or to spawn a mesh slightly offset without needing child entities.

Solution

Add transform_by and transformed_by methods to Mesh. They take a Transform and apply the translation, rotation, and scale to vertex positions, and the rotation to normals and tangents.

In the load_gltf example, with this system:

fn transform(time: Res<Time>, mut q: Query<&mut Handle<Mesh>>, mut meshes: ResMut<Assets<Mesh>>) {
    let sin = 0.0025 * time.elapsed_seconds().sin();

    for mesh_handle in &mut q {
        if let Some(mesh) = meshes.get_mut(mesh_handle.clone_weak()) {
            let transform =
                Transform::from_rotation(Quat::from_rotation_y(0.75 * time.delta_seconds()))
                    .with_scale(Vec3::splat(1.0 + sin));
            mesh.transform_by(transform);
        }
    }
}

it looks like this:

2024-01-21.15-05-43.mp4

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Jan 21, 2024
Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

I like this, but maybe it would be better to implement this as Add<Transform> and AddAssign<Transform>?

@Jondolf Jondolf mentioned this pull request Jan 21, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 21, 2024

I like this, but maybe it would be better to implement this as Add and AddAssign?

Not sure, maybe? But it'd probably be Mul instead of Add. Transformations are typically represented with multiplication, e.g. transform * point is equivalent to transform.transform_point(point); you can't add or subtract transforms.

I could at least add a transformed_by method so that you could create and transform the mesh on a single line instead of having to define a mutable mesh and separately call transform_by on that

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 21, 2024

I ended up adding transformed_by and implementing Mul<Mesh> for Transform. You can now do this:

let transform = Transform::from_xyz(0.0, 2.0, 0.0);

// These are equivalent
let cuboid = Mesh::from(shape::Box::default()).transformed_by(transform);
let cuboid = transform * Mesh::from(shape::Box::default());

let mut cuboid = Mesh::from(shape::Box::default());
cuboid.transform_by(transform);

Feel free to suggest further improvements/changes to the API

@Jondolf Jondolf changed the title Add Mesh::transform_by Add Mesh transformation Jan 21, 2024
@doonv
Copy link
Contributor

doonv commented Jan 21, 2024

Why did you implement Mul<Mesh> for Transform and not Mul<Transform> for Mesh? You could also implement MulAssign<Transform> for Mesh and be able to perform syntax like this:

let mut cuboid = Mesh::from(shape::Box::default());
cuboid *= transform;

// or

let cuboid = Mesh::from(shape::Box::default()) * transform;

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 21, 2024

For consistency. You can't do point * transform or point *= transform, so I don't see why you'd be able to do mesh * transform. Whether or not you should be able to do point * transform is a separate discussion though

For me, the current transform * point or transform * mesh order makes sense, since it reads like "transform point" or "transform mesh". The other way around it'd read something like "to this point, apply transform"

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'd like to see equivalent helpers for the translate/rotate/scale operations alone (that skip the unneeded ops and take in a type-safe arg), but that can be a second PR.

I don't personally think that transformed_by is worth duplicating the API over, but I won't block over it.

@blueforesticarus
Copy link

Here to mention that I ran into exactly this use-case (wanted a xz plane circle).

I do think the Mul<> operator not being symmetric is a bit weird, and I will most likely used the transformed_by variant.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2024
Copy link
Contributor

@NiseVoid NiseVoid left a comment

Choose a reason for hiding this comment

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

Looks good. I agree that separate translate/rotate/scale version later would be a nice followup PR

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

This doesn't transform normals and tangents correctly. Test non-uniform scaling, they need to be component-wise divided by scale in the original unrotated basis and then normalized, because together they make up a bivector.

@atlv24
Copy link
Contributor

atlv24 commented Jan 28, 2024

alternatively to avoid a division, (normal.xyz * scale.yzx * scale.zxy).normalized() (component-wise multiplication)

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2024
@Jondolf Jondolf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2024
@atlv24
Copy link
Contributor

atlv24 commented Jan 29, 2024

If any component of scale is zero it will cause a division by zero and result in infinite-length normals, maybe the division-skipping version that uses only multiplications is a better option

@alice-i-cecile
Copy link
Member

If any component of scale is zero it will cause a division by zero and result in infinite-length normals, maybe the division-skipping version that uses only multiplications is a better option

Won't the multiplication version similarly fail when we attempt to normalize the product? Or is .normalized implicitly "normalize or zero" behavior?

crates/bevy_render/src/mesh/mesh/mod.rs Show resolved Hide resolved
crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 29, 2024

I can use normalize_or_zero for it

@atlv24
Copy link
Contributor

atlv24 commented Jan 29, 2024

No, the multiplication-only version will work fine, i'm talking about only one component being zero. normalize_or_zero is unneeded. edit: if the normal is perpendicular to the axis being flattened along it will be zero. The vector as a whole will usually not be zero as it will still have some non-zero component. It doesnt quite make sense to scale a mesh on two or more axes by zero, that should be guarded for at the top level imo. It would just get collapsed to a line segment and not render anything because all triangles would be zero-area. Zero scales only make sense for flattening meshes along a single axis.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 29, 2024
@atlv24
Copy link
Contributor

atlv24 commented Jan 29, 2024

Thanks for the patience and the fixes

@Jondolf Jondolf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 29, 2024
Merged via the queue into bevyengine:main with commit 70f7d95 Jan 29, 2024
23 checks passed
@Jondolf Jondolf deleted the mesh-transform-by branch January 29, 2024 16:53
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

It can sometimes be useful to transform actual `Mesh` data without
needing to change the `Transform` of an entity. For example, one might
want to spawn a circle mesh facing up instead of facing Z, or to spawn a
mesh slightly offset without needing child entities.

## Solution

Add `transform_by` and `transformed_by` methods to `Mesh`. They take a
`Transform` and apply the translation, rotation, and scale to vertex
positions, and the rotation to normals and tangents.

In the `load_gltf` example, with this system:

```rust
fn transform(time: Res<Time>, mut q: Query<&mut Handle<Mesh>>, mut meshes: ResMut<Assets<Mesh>>) {
    let sin = 0.0025 * time.elapsed_seconds().sin();

    for mesh_handle in &mut q {
        if let Some(mesh) = meshes.get_mut(mesh_handle.clone_weak()) {
            let transform =
                Transform::from_rotation(Quat::from_rotation_y(0.75 * time.delta_seconds()))
                    .with_scale(Vec3::splat(1.0 + sin));
            mesh.transform_by(transform);
        }
    }
}
```

it looks like this:


https://github.com/bevyengine/bevy/assets/57632562/60432456-6d28-4d06-9c94-2f4148f5acd5
Shatur pushed a commit to projectharmonia/bevy that referenced this pull request Feb 12, 2024
# Objective

It can sometimes be useful to transform actual `Mesh` data without
needing to change the `Transform` of an entity. For example, one might
want to spawn a circle mesh facing up instead of facing Z, or to spawn a
mesh slightly offset without needing child entities.

## Solution

Add `transform_by` and `transformed_by` methods to `Mesh`. They take a
`Transform` and apply the translation, rotation, and scale to vertex
positions, and the rotation to normals and tangents.

In the `load_gltf` example, with this system:

```rust
fn transform(time: Res<Time>, mut q: Query<&mut Handle<Mesh>>, mut meshes: ResMut<Assets<Mesh>>) {
    let sin = 0.0025 * time.elapsed_seconds().sin();

    for mesh_handle in &mut q {
        if let Some(mesh) = meshes.get_mut(mesh_handle.clone_weak()) {
            let transform =
                Transform::from_rotation(Quat::from_rotation_y(0.75 * time.delta_seconds()))
                    .with_scale(Vec3::splat(1.0 + sin));
            mesh.transform_by(transform);
        }
    }
}
```

it looks like this:


https://github.com/bevyengine/bevy/assets/57632562/60432456-6d28-4d06-9c94-2f4148f5acd5
github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024
# Objective

It can sometimes be useful to combine several meshes into one. This
allows constructing more complex meshes out of simple primitives without
needing to use a 3D modeling program or entity hierarchies.

This could also be used internally to increase code reuse by using
existing mesh generation logic for e.g. circles and using that in
cylinder mesh generation logic to add the top and bottom of the
cylinder.

**Note**: This is *not* implementing CSGs (Constructive Solid Geometry)
or any boolean operations, as that is much more complex. This is simply
adding the mesh data of another mesh to a mesh.

## Solution

Add a `merge` method to `Mesh`. It appends the vertex attributes and
indices of `other` to `self`, resulting in a `Mesh` that is the
combination of the two.

For example, you could do this:

```rust
let mut cuboid = Mesh::from(shape::Box::default());
let mut cylinder = Mesh::from(shape::Cylinder::default());
let mut torus = Mesh::from(shape::Torus::default());

cuboid.merge(cylinder);
cuboid.merge(torus);
```

This would result in `cuboid` being a `Mesh` that also has the cylinder
mesh and torus mesh. In this case, they would just be placed on top of
each other, but by utilizing #11454 we can transform the cylinder and
torus to get a result like this:


https://github.com/bevyengine/bevy/assets/57632562/557402c6-b896-4aba-bd95-312e7d1b5238

This is just a single entity and a single mesh.
github-merge-queue bot pushed a commit that referenced this pull request Mar 5, 2024
# Objective

Make it straightforward to translate and rotate bounding volumes.

## Solution

Add `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait.
This follows the naming used for mesh transformations (see #11454 and
#11675).

---

## Changelog

- Added `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait
and implemented them for the bounding volumes
- Renamed `Position` associated type to `Translation`

---------

Co-authored-by: Mateusz Wachowiak <[email protected]>
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
# Objective

Make it straightforward to translate and rotate bounding volumes.

## Solution

Add `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait.
This follows the naming used for mesh transformations (see bevyengine#11454 and
bevyengine#11675).

---

## Changelog

- Added `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait
and implemented them for the bounding volumes
- Renamed `Position` associated type to `Translation`

---------

Co-authored-by: Mateusz Wachowiak <[email protected]>
mtsr pushed a commit to mtsr/bevy that referenced this pull request Mar 15, 2024
# Objective

Make it straightforward to translate and rotate bounding volumes.

## Solution

Add `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait.
This follows the naming used for mesh transformations (see bevyengine#11454 and
bevyengine#11675).

---

## Changelog

- Added `translate_by`/`translated_by`, `rotate_by`/`rotated_by`,
`transform_by`/`transformed_by` methods to the `BoundingVolume` trait
and implemented them for the bounding volumes
- Renamed `Position` associated type to `Translation`

---------

Co-authored-by: Mateusz Wachowiak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants