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

bevy_render should not depend on bevy_winit #15565

Closed
alice-i-cecile opened this issue Oct 1, 2024 · 5 comments · Fixed by #15649
Closed

bevy_render should not depend on bevy_winit #15565

alice-i-cecile opened this issue Oct 1, 2024 · 5 comments · Fixed by #15649
Labels
A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Milestone
0.15

Comments

@alice-i-cecile
Copy link
Member

Problem

#14284 added a direct, non-optional dependency on bevy_winit from bevy_render.

This is messy for our crate structure, and makes it much harder to use bevy_render on exotic platforms.

It's also causing issues for contributors:

I'm having issues running tests locally on main. Trying tests like (e.g. cargo test --package bevy_ui --lib layout::ui_surface::tests) I'm getting The platform you're compiling for is not supported by winit

From @StrikeForceZero on Discord.

Proposed solution: revert the cursor icon PR.

Revert #14284 and reopen #14351.

Solves this problem very directly, at the expense of a valuable feature. Back to the drawing board, where none of the options seemed very good.

Proposed solution: make the dependency optional by feature-flagging cursor icons

Still messy, but something that can be worked around. Doesn't lose us the nice feature.

Proposed solution: bevy_cursors crate

Lift this code out into its own crate, which depends on bevy_winit and bevy_render. Probably the cleanest conceptually, but now we have yet another tiny crate to maintain.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through labels Oct 1, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 1, 2024
@alice-i-cecile
Copy link
Member Author

I'm personally okay with any of these solutions, but a bevy_cursors crate is my favorite, followed by the feature flagging approach.

@BenjaminBrienen
Copy link
Contributor

I like the bevy_cursors idea. It's a crate that solves a specific problem and it helps keep the other crates focused on solving their specific problems.

@mockersf
Copy link
Member

mockersf commented Oct 2, 2024

Creating a crate just for cursors feels a bit pointless and like bloat.

I would prefer to move the ANDROID_APP declaration out of bevy_winit. It would make sense to have it in bevy_window for example. that would let us break the dependency between bevy_winit and bevy_asset, and open the possibility to change the direction of dependencies with bevy_render

@alice-i-cecile
Copy link
Member Author

I'm on board with that plan.

@mockersf mockersf mentioned this issue Oct 2, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
@mockersf
move ANDROID_APP to bevy_window (#15585)
23b0dd6 
# Objective

- Remove dependency in bevy_asset to bevy_winit
- First step for #15565 

## Solution

- the static `ANDROID_APP` and the `android_activity` reexport are now
in `bevy_window`

## Migration Guide

If you use the `android_activity` reexport from
`bevy::winit::android_activity`, it is now in
`bevy::window::android_activity`. Same for the `ANDROID_APP` static
robtfm pushed a commit to robtfm/bevy that referenced this issue Oct 4, 2024
@mockersf @robtfm
move ANDROID_APP to bevy_window (bevyengine#15585)
e7826d2 
# Objective

- Remove dependency in bevy_asset to bevy_winit
- First step for bevyengine#15565 

## Solution

- the static `ANDROID_APP` and the `android_activity` reexport are now
in `bevy_window`

## Migration Guide

If you use the `android_activity` reexport from
`bevy::winit::android_activity`, it is now in
`bevy::window::android_activity`. Same for the `ANDROID_APP` static
@KirmesBude KirmesBude mentioned this issue Oct 4, 2024
Merged
@KirmesBude
Copy link
Contributor

Seems like reversing the dependency is not really desirable either.
I can see at least one more use for a bevy_window_extensions crate (something like that, instead of bevy_cursors), which is window icons. Not sure if there is more.

github-merge-queue bot pushed a commit that referenced this issue Oct 6, 2024
@KirmesBude
Inverse bevy_render bevy_winit dependency and move cursor to bevy_win…
6edb78a 
…it (#15649)

# Objective

- `bevy_render` should not depend on `bevy_winit`
- Fixes #15565

## Solution

- `bevy_render` no longer depends on `bevy_winit`
- The following is behind the `custom_cursor` feature
- Move custom cursor code from `bevy_render` to `bevy_winit` behind the
`custom_cursor` feature
- `bevy_winit` now depends on `bevy_render` (for `Image` and
`TextureFormat`)
- `bevy_winit` now depends on `bevy_asset` (for `Assets`, `Handle` and
`AssetId`)
  - `bevy_winit` now depends on `bytemuck` (already in tree)
- Custom cursor code in `bevy_winit` reworked to use `AssetId` (other
than that it is taken over 1:1)
- Rework `bevy_winit` custom cursor interface visibility now that the
logic is all contained in `bevy_winit`

## Testing

- I ran the screenshot and window_settings examples
- Tested on linux wayland so far

---

## Migration Guide

`CursorIcon` and `CustomCursor` previously provided by
`bevy::render::view::cursor` is now available from `bevy::winit`.
A new feature `custom_cursor` enables this functionality (default
feature).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.15
Development

Successfully merging a pull request may close this issue.

Inverse bevy_render bevy_winit dependency and move cursor to bevy_winit KirmesBude/bevy
4 participants
@alice-i-cecile @KirmesBude @mockersf @BenjaminBrienen