Skip to content

Feature gate render in UI (fix bevy_test feature) #16322

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

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Nov 10, 2024

Objective

Fixes #16313 and #16316 and #3815

Solution

Add a lot of feature gates. It looks awful. All configurations build with no warnings, though!

Testing

PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.39s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features               
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.98s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_text
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.28s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_render
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.27s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_render --features bevy_text
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 14.28s

@BenjaminBrienen BenjaminBrienen self-assigned this Nov 10, 2024
@BenjaminBrienen BenjaminBrienen added A-UI Graphical user interfaces, styles, layouts, and widgets D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible labels Nov 10, 2024
@BenjaminBrienen BenjaminBrienen changed the title Feature gate render in UI Feature gate render in UI (fix bevy_test feature) Nov 10, 2024
@BenjaminBrienen
Copy link
Contributor Author

I think it makes sense to also fix bevy_text feature in this PR because there will be a lot of merge conflicts if split.

@BenjaminBrienen BenjaminBrienen added the C-Bug An unexpected or incorrect behavior label Nov 10, 2024
@BenjaminBrienen
Copy link
Contributor Author

After merge:

PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.41s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_text
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.39s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_render
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.37s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_render --features bevy_text
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.15s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 12.60s

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 agree in principle but the diff here is extremely messy and needs to be reduced to a minimal set of changes.

@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
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Nov 10, 2024
@BenjaminBrienen
Copy link
Contributor Author

Latest:
image

@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
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.

I like this change, but I think it needs some polishing. There's some formatting changes that I don't understand the reason for, and I believe reflection was removed from ImageNode inadvertently.

@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 11, 2024
@ickshonpe
Copy link
Contributor

ickshonpe commented Nov 11, 2024

I haven't looked at the changes yet, but I've wanted to have the UI layout and rendering separated for years.
I think just feature gating the rendering isn't enough though, we need the UI rendering plugin moved into its own crate bevy_ui_render or something.

@@ -292,10 +293,11 @@ with UI components as a child of an entity without UI components, your UI layout
}
});

for (camera_id, mut camera) in camera_layout_info.drain() {
for (_camera_id, mut camera) in camera_layout_info.drain() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (_camera_id, mut camera) in camera_layout_info.drain() {
for (camera_id, mut camera) in camera_layout_info.drain() {

Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

There still seems to be something weird with the "bevy_text" feature as well.

Feature gating all the text spawning commands in the testbed_ui example by "bevy_text" and compiling without "bevy_text" enabled:

cargo run --example testbed_ui --no-default-features --features "android-game-activity android-game-activity android_shared_stdcxx animation bevy_asset bevy_audio bevy_color bevy_core_pipeline bevy_gilrs bevy_gizmos bevy_gltf bevy_mesh_picking_backend bevy_pbr bevy_picking bevy_remote bevy_render bevy_scene bevy_sprite bevy_sprite_picking_backend bevy_state bevy_ui bevy_ui_picking_backend bevy_window bevy_winit custom_cursor default_font hdr multi_threaded png smaa_luts sysinfo_plugin tonemapping_luts vorbis webgl2 x11"

results in errors like:

error[E0433]: failed to resolve: use of undeclared type `Text`
  --> examples/testbed/ui.rs:74:33
   |
74 | ...                   Text::new("Text Example"),
   |                       ^^^^ use of undeclared type `Text`

@BenjaminBrienen
Copy link
Contributor Author

I'll look into it, 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 18, 2024
@BenjaminBrienen BenjaminBrienen force-pushed the feature-gate-render-in-ui branch from 87ccfca to b94830d Compare December 26, 2024 21:12
@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 Dec 26, 2024
@BenjaminBrienen
Copy link
Contributor Author

There still seems to be something weird with the "bevy_text" feature as well.

Feature gating all the text spawning commands in the testbed_ui example by "bevy_text" and compiling without "bevy_text" enabled:

cargo run --example testbed_ui --no-default-features --features "android-game-activity android-game-activity android_shared_stdcxx animation bevy_asset bevy_audio bevy_color bevy_core_pipeline bevy_gilrs bevy_gizmos bevy_gltf bevy_mesh_picking_backend bevy_pbr bevy_picking bevy_remote bevy_render bevy_scene bevy_sprite bevy_sprite_picking_backend bevy_state bevy_ui bevy_ui_picking_backend bevy_window bevy_winit custom_cursor default_font hdr multi_threaded png smaa_luts sysinfo_plugin tonemapping_luts vorbis webgl2 x11"

results in errors like:

error[E0433]: failed to resolve: use of undeclared type `Text`
  --> examples/testbed/ui.rs:74:33
   |
74 | ...                   Text::new("Text Example"),
   |                       ^^^^ use of undeclared type `Text`

I get a runtime error if I run this command

@BenjaminBrienen
Copy link
Contributor Author

PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.43s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_render
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.25s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_text  
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.44s
PS C:\Users\BenjaminBrienen\source\bevy> cargo build -p bevy_ui --no-default-features --features bevy_text,bevy_render
   Compiling bevy_ui v0.15.0-dev (C:\Users\BenjaminBrienen\source\bevy\crates\bevy_ui)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 14.08s

@alice-i-cecile alice-i-cecile requested a review from BD103 January 2, 2025 06:11
@BenjaminBrienen BenjaminBrienen force-pushed the feature-gate-render-in-ui branch from 30b7a9d to cc8084c Compare January 3, 2025 21:29
@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 Jan 22, 2025
@alice-i-cecile alice-i-cecile removed this from the 0.16 milestone Mar 16, 2025
@alice-i-cecile
Copy link
Member

Cutting from the milestone: valuable, but not essential. At this point I expect this to be easier to redo from scratch: merge conflicts on this sort of work really really suck.

@BenjaminBrienen BenjaminBrienen added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature-gate render in bevy_ui
4 participants