Open alice-i-cecile opened 1 month ago
If we go with Solution 3... can bevy_asset implement Asset
for Image
instead? This way image is just generic byte storage for images, but in rendering context can also be considered an asset?
Pulling a dependency on bevy_image
from bevy_asset
would be possible, yeah. Still bad, but probably less bad. The end result still isn't very nice though.
2 seems fine to me. bevy_render::Cursor
component is unoptimal but ok when a field on bevy_window::Window
is impossible.
Motivation
Cursor icons (#9557) and window icons (#1031) are highly requested, and seem like they should be easy.
Ideally, we should:
bevy_render
) usingbevy_asset
.winit
viabevy_window
and thenbevy_winit
.Window
, or create an equivalentCursor
component and entity,Straightforward, idiomatic. Like @cart said in https://github.com/bevyengine/bevy/pull/1163#issuecomment-752744609
The problem
Unfortunately, Bevy's multi-crate, modular nature makes this quite painful. This proposed workflow means:
bevy_window
andbevy_winit
need to be aware of theImage
type.bevy_window
andbevy_winit
need to be aware ofHandle
, and thus all ofbevy_asset
.Right now though, the dependencies are reversed:
bevy_render
depends onbevy_window
, because it needs to draw to it.bevy_asset
depends onbevy_winit
to handle an Android edge case.Making
bevy_window
depend onbevy_render
is impossible, since that edge is always required. Makingbevy_winit
depend onbevy_asset
is undesired, since that prevents users from using their own asset-management solution.This was discussed at length in https://github.com/bevyengine/bevy/pull/14284, but has come up in various forms every time a contributor attempts to implement one of these features.
How can we avoid coupling these crates tightly without creating a terrible UX?
Caveat around window icons
As discussed in https://github.com/bevyengine/bevy/pull/8130#issuecomment-2132394328 and other comments in that thread, it's not clear that we can / should be setting the window's icon in Bevy at all. That may be a documentation / build-time template problem.
Solution 1: just use image bytes
Rather than passing in an
Image
type, just pass in a string of bytes in the format thatwinit
expects, as defined by [Icon::from_rgba
].This approach works, and would unlock the features, but ends up feeling very half-baked and unidiomatic. We have an
Image
type, surely we should use that to set images?This solution is the approach taken by #14196, #14284, and #2268. No one seems particularly pleased with the end result, so these PRs stall out repeatedly.
Solution 2: Synchronize with systems
The basic idea here is that we implement a solution like above that just accepts a string of bytes. Then, somewhere in a different bevy crate that can see
bevy_window
,bevy_render
andbevy_asset
, we run a system that tracks a configuration resource or component which stores aHandle<Image>
, and then calls into thebevy_window
API with the new data.We should continue the windows-as-entity pattern here, and use components for this data.
Solution 3: split
Image
into a dedicated crateAs discussed in #11888,
Image
is a type with complex behavior and possibly wide-spread value. If we were to pull it into its own crate, thebevy_window
(and thusbevy_winit
) API could accept a strongly typedImage
, improving the robustness.Unfortunately,
Image
must implementAsset
to be used in Bevy's asset pipeline, and as a result, would end up pulling inbevy_asset
to thebevy_window
tree.This could be avoided using a feature flag, but still means that there's nasty indirection. Without pulling a direct dependency from
bevy_window
tobevy_asset
, this solution is still left with the indirection involved in 2.Analysis
Solution 1 is quick and dirty. The final API is pretty terrible, but it does technically work. This low-level, direct-from-Winit API should probably be exposed no matter what.
Solution 3 doesn't have any serious benefits. To actually get to the ideal solution outlined at the top of this excessively long issue, we would need to pull in
bevy_asset
intobevy_window
, which we don't want to do. Splitting outImage
is also likely to be super controversial and derail things.Solution 2 is weird (why are we updating windowing stuff from
bevy_render
?), but gives a near-ideal API that can use handles to images, and handles the gnarly conversion silently under the hood. It's unfortunate that we can't just have another field onWindow
, or abevy_window::Cursor
component, but without mucking with our crate structure I think this is the best we can do.In theory we could make another crate which depends on everything and stick just the window and cursor handling code in there, but that doesn't make the API any nicer and is more complex and controversial. Simply feature-flagging the section of
bevy_render
seems better.