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

Migrate meshes and materials to required components #15524

Merged
merged 40 commits into from
Oct 1, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Sep 29, 2024

Objective

A big step in the migration to required components: meshes and materials!

Solution

As per the selected proposal:

  • Deprecate MaterialMesh2dBundle, MaterialMeshBundle, and PbrBundle.
  • Add Mesh2d and Mesh3d components, which wrap a Handle<Mesh>.
  • Add MeshMaterial2d<M: Material2d> and MeshMaterial3d<M: Material>, which wrap a Handle<M>.
  • Meshes without a mesh material should be rendered with a default material. The existence of a material is determined by HasMaterial2d/HasMaterial3d, which is required by MeshMaterial2d/MeshMaterial3d. This gets around problems with the generics.

Previously:

commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});

Now:

commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));

If the mesh material is missing, previously nothing was rendered. Now, it renders a white default ColorMaterial in 2D and a StandardMaterial in 3D (this can be overridden). Below, only every other entity has a material:

Näyttökuva 2024-09-29 181746

Näyttökuva 2024-09-29 181918

Why white? This is still open for discussion, but I think white makes sense for a default material, while invalid asset handles pointing to nothing should have something like a pink material to indicate that something is broken (I don't handle that in this PR yet). This is kind of a mix of Godot and Unity: Godot just renders a white material for non-existent materials, while Unity renders nothing when no materials exist, but renders pink for invalid materials. I can also change the default material to pink if that is preferable though.

Testing

I ran some 2D and 3D examples to test if anything changed visually. I have not tested all examples or features yet however. If anyone wants to test more extensively, it would be appreciated!

Implementation Notes

  • The relationship between bevy_render and bevy_pbr is weird here. bevy_render needs Mesh3d for its own systems, but bevy_pbr has all of the material logic, and bevy_render doesn't depend on it. I feel like the two crates should be refactored in some way, but I think that's out of scope for this PR.
  • I didn't migrate meshlets to required components yet. That can probably be done in a follow-up, as this is already a huge PR.
  • It is becoming increasingly clear to me that we really, really want to disallow raw asset handles as components. They caused me a ton of headache here already, and it took me a long time to find every place that queried for them or inserted them directly on entities, since there were no compiler errors for it. If we don't remove the Component derive, I expect raw asset handles to be a huge footgun for users as we transition to wrapper components, especially as handles as components have been the norm so far. I personally consider this to be a blocker for 0.15: we need to migrate to wrapper components for asset handles everywhere, and remove the Component derive. Also see API inconsistency with Handle as component or field of component #14124.

Migration Guide

Asset handles for meshes and mesh materials must now be wrapped in the Mesh2d and MeshMaterial2d or Mesh3d and MeshMaterial3d components for 2D and 3D respectively. Raw handles as components no longer render meshes.

Additionally, MaterialMesh2dBundle, MaterialMeshBundle, and PbrBundle have been deprecated. Instead, use the mesh and material components directly.

Previously:

commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});

Now:

commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));

If the mesh material is missing, a white default material is now used. Previously, nothing was rendered if the material was missing.

The WithMesh2d and WithMesh3d query filter type aliases have also been removed. Simply use With<Mesh2d> or With<Mesh3d>.

@Jondolf Jondolf added A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 29, 2024
@Jondolf Jondolf requested a review from cart September 29, 2024 17:41
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 29, 2024
@Jondolf Jondolf added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 29, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 29, 2024

Hmm one potential problem with the HasMaterial2d/HasMaterial3d approach is that the default material is only used before the first material insertion. If you add a material and then remove it, the default/placeholder material won't be used anymore. We'd need a MaterialCount component that gets incremented/decremented as materials are added/removed, and manage the existence of the marker component based on that.

Should I add this, or is it desired behavior that the placeholder material is only shown before a material is added for the first time?

Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

Reviewed bevy_pbr and bevy_render, still have to check bevy_sprite and the examples

crates/bevy_pbr/src/mesh_material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/mesh_material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/components.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/components.rs Outdated Show resolved Hide resolved
@NiseVoid
Copy link
Contributor

NiseVoid commented Oct 1, 2024

Hmm one potential problem with the HasMaterial2d/HasMaterial3d approach is that the default material is only used before the first material insertion. If you add a material and then remove it, the default/placeholder material won't be used anymore. We'd need a MaterialCount component that gets incremented/decremented as materials are added/removed, and manage the existence of the marker component based on that.

Doesn't seem like the biggest issue at least, the counter would feel more consistent, and not be very hard to make with required components and hooks. But if our plan going forward is to merge Mesh and MeshMaterial in some way that problem would also fix itself, tho that's definitely a more complex topic considering we have features like multiple materials per mesh and whatnot 🤔

@cart cart enabled auto-merge October 1, 2024 21:23
@cart cart added this pull request to the merge queue Oct 1, 2024
Merged via the queue into bevyengine:main with commit 54006b1 Oct 1, 2024
26 checks passed
@Jondolf Jondolf deleted the mesh-required-components-v2 branch October 2, 2024 06:27
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
# Objective

A big step in the migration to required components: meshes and
materials!

## Solution

As per the [selected
proposal](https://hackmd.io/@bevy/required_components/%2Fj9-PnF-2QKK0on1KQ29UWQ):

- Deprecate `MaterialMesh2dBundle`, `MaterialMeshBundle`, and
`PbrBundle`.
- Add `Mesh2d` and `Mesh3d` components, which wrap a `Handle<Mesh>`.
- Add `MeshMaterial2d<M: Material2d>` and `MeshMaterial3d<M: Material>`,
which wrap a `Handle<M>`.
- Meshes *without* a mesh material should be rendered with a default
material. The existence of a material is determined by
`HasMaterial2d`/`HasMaterial3d`, which is required by
`MeshMaterial2d`/`MeshMaterial3d`. This gets around problems with the
generics.

Previously:

```rust
commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});
```

Now:

```rust
commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));
```

If the mesh material is missing, previously nothing was rendered. Now,
it renders a white default `ColorMaterial` in 2D and a
`StandardMaterial` in 3D (this can be overridden). Below, only every
other entity has a material:

![Näyttökuva 2024-09-29
181746](https://github.com/user-attachments/assets/5c8be029-d2fe-4b8c-ae89-17a72ff82c9a)

![Näyttökuva 2024-09-29
181918](https://github.com/user-attachments/assets/58adbc55-5a1e-4c7d-a2c7-ed456227b909)

Why white? This is still open for discussion, but I think white makes
sense for a *default* material, while *invalid* asset handles pointing
to nothing should have something like a pink material to indicate that
something is broken (I don't handle that in this PR yet). This is kind
of a mix of Godot and Unity: Godot just renders a white material for
non-existent materials, while Unity renders nothing when no materials
exist, but renders pink for invalid materials. I can also change the
default material to pink if that is preferable though.

## Testing

I ran some 2D and 3D examples to test if anything changed visually. I
have not tested all examples or features yet however. If anyone wants to
test more extensively, it would be appreciated!

## Implementation Notes

- The relationship between `bevy_render` and `bevy_pbr` is weird here.
`bevy_render` needs `Mesh3d` for its own systems, but `bevy_pbr` has all
of the material logic, and `bevy_render` doesn't depend on it. I feel
like the two crates should be refactored in some way, but I think that's
out of scope for this PR.
- I didn't migrate meshlets to required components yet. That can
probably be done in a follow-up, as this is already a huge PR.
- It is becoming increasingly clear to me that we really, *really* want
to disallow raw asset handles as components. They caused me a *ton* of
headache here already, and it took me a long time to find every place
that queried for them or inserted them directly on entities, since there
were no compiler errors for it. If we don't remove the `Component`
derive, I expect raw asset handles to be a *huge* footgun for users as
we transition to wrapper components, especially as handles as components
have been the norm so far. I personally consider this to be a blocker
for 0.15: we need to migrate to wrapper components for asset handles
everywhere, and remove the `Component` derive. Also see
bevyengine#14124.

---

## Migration Guide

Asset handles for meshes and mesh materials must now be wrapped in the
`Mesh2d` and `MeshMaterial2d` or `Mesh3d` and `MeshMaterial3d`
components for 2D and 3D respectively. Raw handles as components no
longer render meshes.

Additionally, `MaterialMesh2dBundle`, `MaterialMeshBundle`, and
`PbrBundle` have been deprecated. Instead, use the mesh and material
components directly.

Previously:

```rust
commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});
```

Now:

```rust
commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));
```

If the mesh material is missing, a white default material is now used.
Previously, nothing was rendered if the material was missing.

The `WithMesh2d` and `WithMesh3d` query filter type aliases have also
been removed. Simply use `With<Mesh2d>` or `With<Mesh3d>`.

---------

Co-authored-by: Tim Blackbird <justthecooldude@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@Jondolf Jondolf mentioned this pull request Oct 9, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 9, 2024
# Objective

After #15524, there are these bunny-shaped holes in rendering in the
meshlet example!


![broken](https://github.com/user-attachments/assets/9e9f20ec-b820-44df-b961-68a1dee44002)

This is because (1) they're using a raw asset handle instead of
`MeshMaterial3d`, and (2) the system that extracts mesh materials into
the render world has an unnecessary `With<Mesh3d>` filter, which makes
it not account for meshlets.

## Solution

Remove the redundant filter and use `MeshMaterial3d`. The bunnies got
some paint!


![fixed](https://github.com/user-attachments/assets/adb42556-fd4b-4000-8ca8-1356250dd532)
github-merge-queue bot pushed a commit that referenced this pull request Oct 14, 2024
# Objective

Fixes #15847

Alternative to #15862. Would appreciate a rendering person signaling
preference for one or the other.

## Solution

Partially revert the changes made to this example in #15524.

Add comment explaining that the non-usage of the built-in color vertex
attribute is intentional.

## Testing

`cargo run --example mesh2d_manual`
github-merge-queue bot pushed a commit that referenced this pull request Oct 15, 2024
# Objective

Closes #15799.

Many rendering people and maintainers are in favor of reverting default
mesh materials added in #15524, especially as the migration to required
component is already large and heavily breaking.

## Solution

Revert default mesh materials, and adjust docs accordingly.

- Remove `extract_default_materials`
- Remove `clear_material_instances`, and move the logic back into
`extract_mesh_materials`
- Remove `HasMaterial2d` and `HasMaterial3d`
- Change default material handles back to pink instead of white
- 2D uses `Color::srgb(1.0, 0.0, 1.0)`, while 3D uses `Color::srgb(1.0,
0.0, 0.5)`. Not sure if this is intended.

There is now no indication at all about missing materials for `Mesh2d`
and `Mesh3d`. Having a mesh without a material renders nothing.

## Testing

I ran `2d_shapes`, `mesh2d_manual`, and `3d_shapes`, with and without
mesh material components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants