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.

Sign up for GitHub

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

Jump to bottom

Don't reëxport bevy_image from bevy_render #16163

Merged
merged 11 commits into from
Nov 10, 2024
Merged

Don't reëxport bevy_image from bevy_render #16163

merged 11 commits into from
Nov 10, 2024
+189 −170

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Oct 30, 2024 •
edited
Loading

Objective

Fixes #15940

Solution

Remove the pub use and fix the compile errors.
Make bevy_image available as bevy::image.

Testing

Feature Frenzy would be good here! Maybe I'll learn how to use it if I have some time this weekend, or maybe a reviewer can use it.

Migration Guide

Use bevy_image instead of bevy_render::texture items.

@BenjaminBrienen BenjaminBrienen self-assigned this Oct 30, 2024
@BenjaminBrienen BenjaminBrienen added A-Rendering Drawing game state to the screen A-Cross-Cutting Impacts the entire engine D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon labels Oct 30, 2024
bushrat011899
bushrat011899 suggested changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 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! A flatter crate structure will help with compile times and it reduces the coupling between these crates. I do have a suggestion around adding a prelude to bevy_image, so that Image and co. can still be included in the main bevy::prelude, but that's a minor change and would help with the migration guide here too.

crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/mod.rs Show resolved Hide resolved
examples/3d/generate_custom_mesh.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/lib.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 30, 2024
@alice-i-cecile
Copy link
Member

Definitely agree on a prelude + re-exporting. For 0.15, @atlv24 made the decision not to do this to ease migration, but I 100% agree that this is a better design in the long-term.

@BD103
Copy link
Member

BD103 commented Oct 30, 2024

Feature Frenzy would be good here! Maybe I'll learn how to use it if I have some time this weekend, or maybe a reviewer can use it.

flag-frenzy is a bit broken right now because some coupling changes to rendering crates forced x11 or wayland to be specified for more crates than the CI is currently configured for. (And I believe the features are for sub-dependencies, like bevy_render/x11, which flag-frenzy doesn't currently support.)

So feel free to try it! The docs are great, but you may hit a wall or two. (I'll work on improving it once bevy_lint gets off the ground.)

atlv24
atlv24 approved these changes Nov 2, 2024
View reviewed changes
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.

i will admit that i didnt make these changes mostly out of laziness but my excuse is that it kept my diff smaller so it was easier to review and merge. this is the way it should be done. incremental prs are good. thanks!

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 2, 2024
@BenjaminBrienen
Copy link
Contributor Author

I'll address the feedback about the prelude this weekend.

@BenjaminBrienen BenjaminBrienen mentioned this pull request Nov 6, 2024
Open
@BenjaminBrienen BenjaminBrienen force-pushed the Remove-bevy_image-re-exports-from-bevy_render branch from 8f0f225 to 25033d2 Compare November 6, 2024 21:25
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 6, 2024
bushrat011899
bushrat011899 approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Excellent work!

@alice-i-cecile alice-i-cecile 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 Nov 6, 2024
@alice-i-cecile
Copy link
Member

@mockersf want this in 0.15 or 0.16?

@BenjaminBrienen BenjaminBrienen mentioned this pull request Nov 7, 2024
Closed
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 9, 2024
@cart
Copy link
Member

cart commented Nov 9, 2024

Just pushed some minor changes:

  1. Removed a lot of the prelude usage in places where we prefer direct imports
  2. Removed Volume from prelude. The original issue was more concerned with Volume not being made available at all not that it needs prelude membership. It being exported from bevy_image in the normal spot is good enough.
  3. Removed a couple of unnecessary imports

@cart
Copy link
Member

cart commented Nov 9, 2024

  1. I also removed a couple of instances of "feature gated imports". They had "redundant imports" in them, and I generally think its better to just specify full paths directly where they are used.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 9, 2024
@BenjaminBrienen BenjaminBrienen mentioned this pull request Nov 9, 2024
Open
mockersf
mockersf requested changes Nov 10, 2024
View reviewed changes
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

bevy_image is optional so it needs to be feature gated

crates/bevy_internal/src/lib.rs Show resolved Hide resolved
crates/bevy_internal/src/prelude.rs Outdated Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 10, 2024
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 10, 2024
@mockersf mockersf added this pull request to the merge queue Nov 10, 2024
Merged via the queue into bevyengine:main with commit 40640fd Nov 10, 2024
34 checks passed
@BenjaminBrienen BenjaminBrienen deleted the Remove-bevy_image-re-exports-from-bevy_render branch November 10, 2024 07:52
@BenjaminBrienen BenjaminBrienen removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Nov 10, 2024
mockersf pushed a commit that referenced this pull request Nov 11, 2024
@BenjaminBrienen @chompaa
@cart @mockersf
Don't reëxport bevy_image from bevy_render (#16163)
31ec575 
Fixes #15940

Remove the `pub use` and fix the compile errors.
Make `bevy_image` available as `bevy::image`.

Feature Frenzy would be good here! Maybe I'll learn how to use it if I
have some time this weekend, or maybe a reviewer can use it.

Use `bevy_image` instead of `bevy_render::texture` items.

---------

Co-authored-by: chompaa <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@cart cart mentioned this pull request Nov 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlv24 atlv24 atlv24 approved these changes

@mockersf mockersf mockersf approved these changes

@cart cart cart approved these changes

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@alice-i-cecile alice-i-cecile Awaiting requested review from alice-i-cecile

@BD103 BD103 Awaiting requested review from BD103

Assignees

@BenjaminBrienen BenjaminBrienen

Labels
A-Cross-Cutting Impacts the entire engine A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
Rendering
Status: Done
Milestone
0.15
Development

Successfully merging this pull request may close these issues.

Remove bevy_image re-exports from bevy_render
8 participants
@BenjaminBrienen @alice-i-cecile @BD103 @cart @bushrat011899 @mockersf @atlv24 @chompaa