Skip to content

Revert default mesh materials#15930

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
Jondolf:revert-default-materials
Oct 15, 2024
Merged

Revert default mesh materials#15930
alice-i-cecile merged 1 commit intobevyengine:mainfrom
Jondolf:revert-default-materials

Conversation

@Jondolf
Copy link
Copy Markdown
Contributor

@Jondolf Jondolf commented Oct 15, 2024 •
edited
Loading

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.

@Jondolf Jondolf added A-Rendering Drawing game state to the screen X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 15, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills labels Oct 15, 2024
@Jondolf Jondolf added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
ecoskey
ecoskey approved these changes Oct 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ecoskey ecoskey left a comment

Choose a reason for hiding this comment

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

Have we decided on not needing/wanting a debug warning at least? Otherwise LGTM

@Jondolf
Copy link
Copy Markdown
Contributor Author

Jondolf commented Oct 15, 2024

A warning for missing materials would probably require adding HasMaterial2d/HasMaterial3d again, and my understanding was that we want to remove it. And custom rendering solutions (which I believe were one reason for wanting to remove default materials) would then also need to deal with the warnings.

@Jondolf Jondolf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 15, 2024
Merged via the queue into bevyengine:main with commit c1a4b82 Oct 15, 2024
@Jondolf Jondolf deleted the revert-default-materials branch October 19, 2024 18:11
@baldraven baldraven mentioned this pull request Nov 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@tychedelia tychedelia tychedelia approved these changes

@ecoskey ecoskey ecoskey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers

Projects

No open projects
Rendering (Old)
Status: Done

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

Revert default mesh material

4 participants

@Jondolf @tychedelia @ecoskey @alice-i-cecile