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.

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

Remove or shrink bevy_utils #11478

Open
5 of 16 tasks
doonv opened this issue Jan 22, 2024 · 16 comments
Open
5 of 16 tasks

Remove or shrink bevy_utils #11478

doonv opened this issue Jan 22, 2024 · 16 comments
Labels
A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change

Comments

@doonv
Copy link
Contributor

doonv commented Jan 22, 2024

What problem does this solve or what need does it fill?

bevy_utils is a collection of unrelated utilities for Bevy. Utility modules/classes/libraries are generally frowned upon in the programming community since they are most likely a sign of poor organization.

What solution would you like?

Move bevy_utils's contents into existing bevy crates or into separate crates.

Long list of what to do with each item in bevy_utils

  • BoxedFuture could probably be removed since async in traits was added in Rust 1.75
  • all HashMap related stuff: no idea where to put that.
  • Move EntityHash related types into bevy_ecs #11498
  • OnDrop could be its own crate
  • tracing related stuff: put in bevy_log
  • once related stuff: I would put it in bevy_log, but this could cause some issues (see move once from bevy_log to bevy_utils, to allow for it's use in bevy_ecs #11419)
  • get_short_name: put in bevy_reflect
  • SyncCell: use std::sync::Exclusive when stabilized
  • SyncUnsafeCell: use std::cell::SyncUnsafeCell when stablized
  • generate_composite_uuid: Maybe contribute this to the uuid crate?
  • label module: Sounds like a bevy_reflect thing but isn't actually used anywhere there so idk
  • Interned: put in bevy_ecs
  • futures module: only used in bevy_render, so put it there.
  • FloatOrd: could be its own crate
  • default: Could be its own crate (The default crate already exists, so maybe we should use that)
  • CowArc: Could be its own crate
@doonv doonv added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jan 22, 2024
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Utils Utility functions and types and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jan 22, 2024
@alice-i-cecile
Copy link
Member

Yes please: I want this crate eliminated!

@alice-i-cecile
Copy link
Member

I think we may want to spin up new crates in the Bevy org for some of the stand-alone utilities.

@cart, would that make sense to you? Things like the default function crate would be good to split out, and will change slowly enough that I don't have concerns about moving it out of the monorepo.

That said, I know you're reluctant about increasing crate number, just because it's a popular misguided punching bag for "dependency bloat".

@doonv
Copy link
Contributor Author

doonv commented Jan 22, 2024

I'm curious whats the compile-time performance penalty of switching between compiling different crates vs. the compile-time performance penalty of compiling unused parts of a crate.

@hymm
Copy link
Contributor

hymm commented Jan 22, 2024

label is used in bevy_ecs to define ScheduleLabel trait and SystemSet trait. So maybe should be there.

get_short_name is used in bevy_asset in a Debug impl, bevy_ecs for some ambiguity reporting, and bevy_heirarchy for a warning. So I don't think it makes sense in bevy_reflect which is an optional dependency for those crates. Not sure where it should go though.

@alice-i-cecile
Copy link
Member

get_short_name feels like a general purpose Rust utility, or maybe something that belongs in bevy_reflect for working with types and paths.

github-merge-queue bot pushed a commit that referenced this issue Feb 3, 2024
Use `TypeIdMap<T>` instead of `HashMap<TypeId, T>`

- ~~`TypeIdMap` was in `bevy_ecs`. I've kept it there because of
#11478~~
- ~~I haven't swapped `bevy_reflect` over because it doesn't depend on
`bevy_ecs`, but I'd also be happy with moving `TypeIdMap` to
`bevy_utils` and then adding a dependency to that~~
- ~~this is a slight change in the public API of
`DrawFunctionsInternal`, does this need to go in the changelog?~~

## Changelog
- moved `TypeIdMap` to `bevy_utils`
- changed `DrawFunctionsInternal::indices` to `TypeIdMap`

## Migration Guide

- `TypeIdMap` now lives in `bevy_utils`
- `DrawFunctionsInternal::indices` now uses a `TypeIdMap`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
tjamaan pushed a commit to tjamaan/bevy that referenced this issue Feb 6, 2024
Use `TypeIdMap<T>` instead of `HashMap<TypeId, T>`

- ~~`TypeIdMap` was in `bevy_ecs`. I've kept it there because of
bevyengine#11478~~
- ~~I haven't swapped `bevy_reflect` over because it doesn't depend on
`bevy_ecs`, but I'd also be happy with moving `TypeIdMap` to
`bevy_utils` and then adding a dependency to that~~
- ~~this is a slight change in the public API of
`DrawFunctionsInternal`, does this need to go in the changelog?~~

## Changelog
- moved `TypeIdMap` to `bevy_utils`
- changed `DrawFunctionsInternal::indices` to `TypeIdMap`

## Migration Guide

- `TypeIdMap` now lives in `bevy_utils`
- `DrawFunctionsInternal::indices` now uses a `TypeIdMap`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
# Objective

Reduce the size of `bevy_utils`
(#11478)

## Solution

Move `EntityHash` related types into `bevy_ecs`. This also allows us
access to `Entity`, which means we no longer need `EntityHashMap`'s
first generic argument.

---

## Changelog

- Moved `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` into `bevy::ecs::entity::hash` .
- Removed `EntityHashMap`'s first generic argument. It is now hardcoded
to always be `Entity`.

## Migration Guide

- Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` now have to be imported from `bevy::ecs::entity::hash`.
- Uses of `EntityHashMap` no longer have to specify the first generic
parameter. It is now hardcoded to always be `Entity`.
Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this issue Feb 20, 2024
# Objective

Reduce the size of `bevy_utils`
(bevyengine/bevy#11478)

## Solution

Move `EntityHash` related types into `bevy_ecs`. This also allows us
access to `Entity`, which means we no longer need `EntityHashMap`'s
first generic argument.

---

## Changelog

- Moved `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` into `bevy::ecs::entity::hash` .
- Removed `EntityHashMap`'s first generic argument. It is now hardcoded
to always be `Entity`.

## Migration Guide

- Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` now have to be imported from `bevy::ecs::entity::hash`.
- Uses of `EntityHashMap` no longer have to specify the first generic
parameter. It is now hardcoded to always be `Entity`.
@Kanabenki
Copy link
Contributor

Kanabenki commented Feb 29, 2024

Regarding the tracing utiliities in bevy_utils, I checked whether that could be easily moved to bevy_log, but that creates a deps cycle since it already depends on bevy_app and bevy_ecs because of the LogPlugin. Would it make sense to have a bevy_tracing crate just for re-exporting tracing + the few tracing utilities?

github-merge-queue bot pushed a commit that referenced this issue Mar 7, 2024
# Objective
Make bevy_utils less of a compilation bottleneck. Tackle #11478.

## Solution
* Move all of the directly reexported dependencies and move them to
where they're actually used.
* Remove the UUID utilities that have gone unused since `TypePath` took
over for `TypeUuid`.
* There was also a extraneous bytemuck dependency on `bevy_core` that
has not been used for a long time (since `encase` became the primary way
to prepare GPU buffers).
* Remove the `all_tuples` macro reexport from bevy_ecs since it's
accessible from `bevy_utils`.

---

## Changelog
Removed: Many of the reexports from bevy_utils (petgraph, uuid, nonmax,
smallvec, and thiserror).
Removed: bevy_core's reexports of bytemuck.

## Migration Guide
bevy_utils' reexports of petgraph, uuid, nonmax, smallvec, and thiserror
have been removed.

bevy_core' reexports of bytemuck's types has been removed. 

Add them as dependencies in your own crate instead.
spectria-limina pushed a commit to spectria-limina/bevy that referenced this issue Mar 9, 2024
…e#12313)

# Objective
Make bevy_utils less of a compilation bottleneck. Tackle bevyengine#11478.

## Solution
* Move all of the directly reexported dependencies and move them to
where they're actually used.
* Remove the UUID utilities that have gone unused since `TypePath` took
over for `TypeUuid`.
* There was also a extraneous bytemuck dependency on `bevy_core` that
has not been used for a long time (since `encase` became the primary way
to prepare GPU buffers).
* Remove the `all_tuples` macro reexport from bevy_ecs since it's
accessible from `bevy_utils`.

---

## Changelog
Removed: Many of the reexports from bevy_utils (petgraph, uuid, nonmax,
smallvec, and thiserror).
Removed: bevy_core's reexports of bytemuck.

## Migration Guide
bevy_utils' reexports of petgraph, uuid, nonmax, smallvec, and thiserror
have been removed.

bevy_core' reexports of bytemuck's types has been removed. 

Add them as dependencies in your own crate instead.
github-merge-queue bot pushed a commit that referenced this issue Apr 8, 2024
# Objective

- Attempts to solve two items from
#11478.

## Solution

- Moved `intern` module from `bevy_utils` into `bevy_ecs` crate and
updated all relevant imports.
- Moved `label` module from `bevy_utils` into `bevy_ecs` crate and
updated all relevant imports.

---

## Migration Guide

- Replace `bevy_utils::define_label` imports with
`bevy_ecs::define_label` imports.
- Replace `bevy_utils::label::DynEq` imports with
`bevy_ecs::label::DynEq` imports.
- Replace `bevy_utils::label::DynHash` imports with
`bevy_ecs::label::DynHash` imports.
- Replace `bevy_utils::intern::Interned` imports with
`bevy_ecs::intern::Interned` imports.
- Replace `bevy_utils::intern::Internable` imports with
`bevy_ecs::intern::Internable` imports.
- Replace `bevy_utils::intern::Interner` imports with
`bevy_ecs::intern::Interner` imports.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
@mnmaita
Copy link
Member

mnmaita commented Apr 26, 2024

label and intern modules were moved into bevy_ecs. You can tick those whenever you can. I might work on some other items on this list later. Thanks!

@mnmaita
Copy link
Member

mnmaita commented May 23, 2024

FloatOrd can be ticked as it was moved here.

Regarding stuff like CowArc and OnDrop, does it make sense to have these as crates within the bevy repo, or would it make more sense to have them as standalone repositories within the org?

@alice-i-cecile
Copy link
Member

Those should be in the org in their own repo IMO, and not Bevy branded.

@mnmaita
Copy link
Member

mnmaita commented May 24, 2024

Those should be in the org in their own repo IMO, and not Bevy branded.

100% agree. I could tackle moving CowArc out of bevy but I guess I would need someone to create the repository (I think?).

@mnmaita
Copy link
Member

mnmaita commented May 24, 2024

https://crates.io/crates/cow_arc this name is taken FWIW.

@mnmaita
Copy link
Member

mnmaita commented May 28, 2024

I'm currently working on bevy_tracing. I'll try to push a draft sometime this week.

@mintlu8
Copy link
Contributor

mintlu8 commented May 28, 2024

About HashMap, I suggest kill it completely and replace with rustc_hash::FxHashMap.

@tsal
Copy link

tsal commented Jun 15, 2024

did bevy::utils::thiserror get removed / moved as part of this effort in 0.14? I can't seem to find the module anymore, but that could be user error (no pun intended).

@BD103
Copy link
Member

BD103 commented Jun 15, 2024

did bevy::utils::thiserror get removed / moved as part of this effort in 0.14? I can't seem to find the module anymore, but that could be user error (no pun intended).

I believe it did. You can view the draft migration guide here.

@mnmaita
Copy link
Member

mnmaita commented Jun 15, 2024

BoxedFuture usage was reduced in #12550 but it still exists. Not sure if there will be a way to completely remove it though (check the convo on that PR for more context).

dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this issue Jul 7, 2024
# Objective

Reduce the size of `bevy_utils`
(bevyengine/bevy#11478)

## Solution

Move `EntityHash` related types into `bevy_ecs`. This also allows us
access to `Entity`, which means we no longer need `EntityHashMap`'s
first generic argument.

---

## Changelog

- Moved `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` into `bevy::ecs::entity::hash` .
- Removed `EntityHashMap`'s first generic argument. It is now hardcoded
to always be `Entity`.

## Migration Guide

- Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` now have to be imported from `bevy::ecs::entity::hash`.
- Uses of `EntityHashMap` no longer have to specify the first generic
parameter. It is now hardcoded to always be `Entity`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

No branches or pull requests

8 participants