Commit graph

7413 commits

Author SHA1 Message Date
Liam Gallagher
26808745cf
Fix bevy_window and bevy_winit readme badges (#15637)
## Objective

Fix the badges for `bevy_window` and `bevy_winit`.

## Solution

Replace the placeholder `bevy_name` wit the correct crate name.
2024-10-04 00:38:49 +00:00
fluffiac
f0704cffa4
Allow a closure to be used as a required component default (#15269)
# Objective

Allow required component default values to be provided in-line.

```rust
#[derive(Component)]
#[require(
    FocusPolicy(block_focus_policy)
)]
struct SomeComponent;

fn block_focus_policy() -> FocusPolicy {
    FocusPolicy::Block
}
```

May now be expressed as:

```rust
#[derive(Component)]
#[require(
    FocusPolicy(|| FocusPolicy::Block)
)]
struct SomeComponent;
```

## Solution

Modified the #[require] proc macro to accept a closure. 

## Testing

Tested using my branch as a dependency, and switching between the inline
closure syntax and function syntax for a bunch of different components.
2024-10-04 00:34:39 +00:00
Tim
20dbf790a6
Get rid of unnecessary mutable access in ui picking backend (#15630)
## Solution

Yeet

## Testing

Tested the `simple_picking` example
2024-10-03 21:30:52 +00:00
rewin
8bf5d99d86
Add method to remove component and all required components for removed component (#15026)
## Objective
The new Required Components feature (#14791) in Bevy allows spawning a
fixed set of components with a single method with cool require macro.
However, there's currently no corresponding method to remove all those
components together. This makes it challenging to keep insertion and
removal code in sync, especially for simple using cases.
```rust
#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
struct Y;

world.entity_mut(e).insert(X); // Spawns both X and Y
world.entity_mut(e).remove::<X>(); 
world.entity_mut(e).remove::<Y>(); // We need to manually remove dependencies without any sync with the `require` macro
```
## Solution
Simplifies component management by providing operations for removal
required components.
This PR introduces simple 'footgun' methods to removes all components of
this bundle and its required components.

Two new methods are introduced:
For Commands:
```rust
commands.entity(e).remove_with_requires::<B>();
```
For World:
```rust
world.entity_mut(e).remove_with_requires::<B>();
```

For performance I created new field in Bundels struct. This new field
"contributed_bundle_ids" contains cached ids for dynamic bundles
constructed from bundle_info.cintributed_components()

## Testing
The PR includes three test cases:

1. Removing a single component with requirements using World.
2. Removing a bundle with requirements using World.
3. Removing a single component with requirements using Commands.
4. Removing a single component with **runtime** requirements using
Commands

These tests ensure the feature works as expected across different
scenarios.

## Showcase
Example:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
#[require(Z)]
struct Y;

#[derive(Component, Default)]
struct Z;

#[derive(Component)]
struct W;

let mut world = World::new();

// Spawn an entity with X, Y, Z, and W components
let entity = world.spawn((X, W)).id();

assert!(world.entity(entity).contains::<X>());
assert!(world.entity(entity).contains::<Y>());
assert!(world.entity(entity).contains::<Z>());
assert!(world.entity(entity).contains::<W>());

// Remove X and required components Y, Z
world.entity_mut(entity).remove_with_requires::<X>();

assert!(!world.entity(entity).contains::<X>());
assert!(!world.entity(entity).contains::<Y>());
assert!(!world.entity(entity).contains::<Z>());

assert!(world.entity(entity).contains::<W>());
```

## Motivation for PR
#15580 

## Performance

I made simple benchmark
```rust
let mut world = World::default();
let entity = world.spawn_empty().id();

let steps = 100_000_000;

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove::<(X, Y, Z, W)>();
}
let end = std::time::Instant::now();
println!("normal remove: {:?} ", (end - start).as_secs_f32());
println!("one remove: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove_with_requires::<X>();
}
let end = std::time::Instant::now();
println!("remove_with_requires: {:?} ", (end - start).as_secs_f32());
println!("one remove_with_requires: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);
```

Output:

CPU: Amd Ryzen 7 2700x

```bash
normal remove: 17.36135 
one remove: 0.17361348299999999 micros
remove_with_requires: 17.534006 
one remove_with_requires: 0.17534005400000002 micros
```

NOTE: I didn't find any tests or mechanism in the repository to update
BundleInfo after creating new runtime requirements with an existing
BundleInfo. So this PR also does not contain such logic.

## Future work (outside this PR)

Create cache system for fast removing components in "safe" mode, where
"safe" mode is remove only required components that will be no longer
required after removing root component.

---------

Co-authored-by: a.yamaev <a.yamaev@smartengines.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
2024-10-03 20:35:08 +00:00
IceSentry
0628255c45
send_events is ambiguous_with_all (#15629)
# Objective

> Alice 🌹 — Today at 3:43 PM
bevy_dev_tools::ci_testing::systems::send_events
This system should be marked as ambiguous with everything I think

## Solution

- Mark it as `ambiguous_with_all`
2024-10-03 20:02:52 +00:00
Matty
528ca4f95e
Eliminate redundant clamping from sample-interpolated curves (#15620)
# Objective

Currently, sample-interpolated curves (such as those used by the glTF
loader for animations) do unnecessary extra work when `sample_clamped`
is called, since their implementations of `sample_unchecked` are already
clamped. Eliminating this redundant sampling is a small, easy
performance win which doesn't compromise on the animation system's
internal usage of `sample_clamped`, which guarantees that it never
samples curves out-of-bounds.

## Solution

For sample-interpolated curves, define `sample_clamped` in the way
`sample_unchecked` is currently defined, and then redirect
`sample_unchecked` to `sample_clamped`. This is arguably a more
idiomatic way of using the `cores` as well, which is nice.

## Testing

Ran `many_foxes` to make sure I didn't break anything.
2024-10-03 18:26:41 +00:00
Chris Russell
46180a75f8
System param for dynamic resources (#15189)
# Objective

Support accessing dynamic resources in a dynamic system, including
accessing them by component id. This is similar to how dynamic
components can be queried using `Query<FilteredEntityMut>`.

## Solution

Create `FilteredResources` and `FilteredResourcesMut` types that act
similar to `FilteredEntityRef` and `FilteredEntityMut` and that can be
used as system parameters.

## Example

```rust
// Use `FilteredResourcesParamBuilder` to declare access to resources.
let system = (FilteredResourcesParamBuilder::new(|builder| {
    builder.add_read::<B>().add_read::<C>();
}),)
    .build_state(&mut world)
    .build_system(resource_system);

world.init_resource::<A>();
world.init_resource::<C>();

fn resource_system(res: FilteredResources) {
    // The resource exists, but we have no access, so we can't read it.
    assert!(res.get::<A>().is_none());
    // The resource doesn't exist, so we can't read it.
    assert!(res.get::<B>().is_none());
    // The resource exists and we have access, so we can read it.
    let c = res.get::<C>().unwrap();
    // The type parameter can be left out if it can be determined from use.
    let c: Res<C> = res.get().unwrap();
}
```

## Future Work

As a follow-up PR, `ReflectResource` can be modified to take `impl
Into<FilteredResources>`, similar to how `ReflectComponent` takes `impl
Into<FilteredEntityRef>`. That will allow dynamic resources to be
accessed using reflection.
2024-10-03 18:20:34 +00:00
ickshonpe
1e61092604
Fix extract_text2d_sprite entity leak (#15625)
# Objective

`extract_2d_sprite` still uses `spawn_empty()`, replace with
`spawn(TemporaryRenderEntity)` .
2024-10-03 18:15:36 +00:00
ickshonpe
9bb27e97c5
Fix entity leak in extract_uinode_borders (#15626)
# Objective

Fix for another leak, this time when extracting outlines.
2024-10-03 18:15:32 +00:00
Kristoffer Søholm
336c23c1aa
Rename observe to observe_entity on EntityWorldMut (#15616)
# Objective

The current observers have some unfortunate footguns where you can end
up confused about what is actually being observed. For apps you can
chain observe like `app.observe(..).observe(..)` which works like you
would expect, but if you try the same with world the first `observe()`
will return the `EntityWorldMut` for the created observer, and the
second `observe()` will only observe on the observer entity. It took
several hours for multiple people on discord to figure this out, which
is not a great experience.

## Solution

Rename `observe` on entities to `observe_entity`. It's slightly more
verbose when you know you have an entity, but it feels right to me that
observers for specific things have more specific naming, and it prevents
this issue completely.

Another possible solution would be to unify `observe` on `App` and
`World` to have the same kind of return type, but I'm not sure exactly
what that would look like.

## Testing

Simple name change, so only concern is docs really.

---


## Migration Guide

The `observe()` method on entities has been renamed to
`observe_entity()` to prevent confusion about what is being observed in
some cases.
2024-10-03 17:05:26 +00:00
rudderbucky
2da8d17a44
Add try_despawn methods to World/Commands (#15480)
# Objective

Fixes #14511.

`despawn` allows you to remove entities from the world. However, if the
entity does not exist, it emits a warning. This may not be intended
behavior for many users who have use cases where they need to call
`despawn` regardless of if the entity actually exists (see the issue),
or don't care in general if the entity already doesn't exist.

(Also trying to gauge interest on if this feature makes sense, I'd
personally love to have it, but I could see arguments that this might be
a footgun. Just trying to help here 😄 If there's no contention I could
also implement this for `despawn_recursive` and `despawn_descendants` in
the same PR)

## Solution

Add `try_despawn`, `try_despawn_recursive` and
`try_despawn_descendants`.

Modify `World::despawn_with_caller` to also take in a `warn` boolean
argument, which is then considered when logging the warning. Set
`log_warning` to `true` in the case of `despawn`, and `false` in the
case of `try_despawn`.

## Testing

Ran `cargo run -p ci` on macOS, it seemed fine.
2024-10-03 16:21:05 +00:00
rudderbucky
5e81154e9c
Despawn and despawn_recursive benchmarks (#15610)
# Objective

Add despawn and despawn_recursive benchmarks in a similar vein to the
spawn benchmark.

## Testing

Ran `cargo bench` from `benches` and it compiled fine.

On my machine:
```
despawn_world/1_entities
                        time:   [3.1495 ns 3.1574 ns 3.1652 ns]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe
despawn_world/10_entities
                        time:   [28.629 ns 28.674 ns 28.720 ns]
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
despawn_world/100_entities
                        time:   [286.95 ns 287.41 ns 287.90 ns]
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild
despawn_world/1000_entities
                        time:   [2.8739 µs 2.9001 µs 2.9355 µs]
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) high mild
  6 (6.00%) high severe
despawn_world/10000_entities
                        time:   [28.535 µs 28.617 µs 28.698 µs]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

despawn_world_recursive/1_entities
                        time:   [5.2270 ns 5.2507 ns 5.2907 ns]
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low mild
  6 (6.00%) high mild
  4 (4.00%) high severe
despawn_world_recursive/10_entities
                        time:   [57.495 ns 57.590 ns 57.691 ns]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
despawn_world_recursive/100_entities
                        time:   [514.43 ns 518.91 ns 526.88 ns]
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe
despawn_world_recursive/1000_entities
                        time:   [5.0362 µs 5.0463 µs 5.0578 µs]
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) high mild
  5 (5.00%) high severe
despawn_world_recursive/10000_entities
                        time:   [51.159 µs 51.603 µs 52.215 µs]
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) high mild
  6 (6.00%) high severe
```
2024-10-03 14:59:37 +00:00
MiniaczQ
acea4e7e6f
Better warnings about invalid parameters (#15500)
# Objective

System param validation warnings should be configurable and default to
"warn once" (per system).

Fixes: #15391

## Solution

`SystemMeta` is given a new `ParamWarnPolicy` field.
The policy decides whether warnings will be emitted by each system param
when it fails validation.
The policy is updated by the system after param validation fails.

Example warning:
```
2024-09-30T18:10:04.740749Z  WARN bevy_ecs::system::function_system: System fallible_params::do_nothing_fail_validation will not run because it requested inaccessible system parameter Single<(), (With<Player>, With<Enemy>)>
```

Currently, only the first invalid parameter is displayed.

Warnings can be disabled on function systems using
`.param_never_warn()`.
(there is also `.with_param_warn_policy(policy)`)

## Testing

Ran `fallible_params` example.

---------

Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net>
2024-10-03 13:16:55 +00:00
Patrick Walton
ca8dd06146
Impose a more sensible ordering for animation graph evaluation. (#15589)
This is an updated version of #15530. Review comments were addressed.

This commit changes the animation graph evaluation to be operate in a
more sensible order and updates the semantics of blend nodes to conform
to [the animation composition RFC]. Prior to this patch, a node graph
like this:

```
	    ┌─────┐
	    │     │
	    │  1  │
	    │     │
	    └──┬──┘
	       │
       ┌───────┴───────┐
       │               │
       ▼               ▼
    ┌─────┐         ┌─────┐
    │     │         │     │
    │  2  │         │  3  │
    │     │         │     │
    └──┬──┘         └──┬──┘
       │               │
   ┌───┴───┐       ┌───┴───┐
   │       │       │       │
   ▼       ▼       ▼       ▼
┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐
│     │ │     │ │     │ │     │
│  4  │ │  6  │ │  5  │ │  7  │
│     │ │     │ │     │ │     │
└─────┘ └─────┘ └─────┘ └─────┘
```

Would be evaluated as (((4 ⊕ 5) ⊕ 6) ⊕ 7), with the blend (lerp/slerp)
operation notated as ⊕. As quaternion multiplication isn't commutative,
this is very counterintuitive and will especially lead to trouble with
the forthcoming additive blending feature (#15198).

This patch fixes the issue by changing the evaluation order to
postorder, with children of a node evaluated in ascending order by node
index.

To do so, this patch revamps `AnimationCurve` to be based on an
*evaluation stack* and a *blend register*. During target evaluation, the
graph evaluator traverses the graph in postorder. When encountering a
clip node, the evaluator pushes the possibly-interpolated value onto the
evaluation stack. When encountering a blend node, the evaluator pops
values off the stack into the blend register, accumulating weights as
appropriate. When the graph is completely evaluated, the top element on
the stack is *committed* to the property of the component.

A new system, the *graph threading* system, is added in order to cache
the sorted postorder traversal to avoid the overhead of sorting children
at animation evaluation time. Mask evaluation has been moved to this
system so that the graph only has to be traversed at most once per
frame. Unlike the `ActiveAnimation` list, the *threaded graph* is cached
from frame to frame and only has to be regenerated when the animation
graph asset changes.

This patch currently regresses the `animate_target` performance in
`many_foxes` by around 50%, resulting in an FPS loss of about 2-3 FPS.
I'd argue that this is an acceptable price to pay for a much more
intuitive system. In the future, we can mitigate the regression with a
fast path that avoids consulting the graph if only one animation is
playing. However, in the interest of keeping this patch simple, I didn't
do so here.

[the animation composition RFC]:
https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md

# Objective

- Describe the objective or issue this PR addresses.
- If you're fixing a specific issue, say "Fixes #X".

## Solution

- Describe the solution used to achieve the objective above.

## Testing

- Did you test these changes? If so, how?
- Are there any parts that need more testing?
- How can other people (reviewers) test your changes? Is there anything
specific they need to know?
- If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?

---

## Showcase

> This section is optional. If this PR does not include a visual change
or does not add a new feature, you can delete this section.

- Help others understand the result of this PR by showcasing your
awesome work!
- If this PR adds a new feature or public API, consider adding a brief
pseudo-code snippet of it in action
- If this PR includes a visual change, consider adding a screenshot,
GIF, or video
  - If you want, you could even include a before/after comparison!
- If the Migration Guide adequately covers the changes, you can delete
this section

While a showcase should aim to be brief and digestible, you can use a
toggleable section to save space on longer showcases:

<details>
  <summary>Click to view showcase</summary>

```rust
println!("My super cool code.");
```

</details>

## Migration Guide

> This section is optional. If there are no breaking changes, you can
delete this section.

- If this PR is a breaking change (relative to the last release of
Bevy), describe how a user might need to migrate their code to support
these changes
- Simply adding new functionality is not a breaking change.
- Fixing behavior that was definitely a bug, rather than a questionable
design choice is not a breaking change.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
2024-10-03 00:36:42 +00:00
IceSentry
67744bb011
Use circle gizmos for capsule (#15602)
# Objective

- The capsule gizmo uses straight lines for the upper and lower circle
which looks pretty ugly.

## Solution

- Use the circle gizmo instead

---

## Showcase

**BEFORE**

![3d_gizmos_sy3CmKUvKO](https://github.com/user-attachments/assets/be014de4-751e-4b40-b814-b5b97bb72031)

**AFTER**

![3d_gizmos_nyADBAUJHg](https://github.com/user-attachments/assets/539ff765-f9d8-4afe-9ac6-41fe83e94e94)

(the circles are red for demonstration purposes only)

# Notes

I also tried using 3d arcs instead of circles but it looks like arcs
need a lot more computation for an almost identical end result. Circles
seem much simpler. The only thing I'm unsure about is if the rotation
stuff is correct. It worked in my testing though.
2024-10-02 19:47:56 +00:00
Matt Tracy
8fb55dbf59
Implement SystemParam::queue() method for blanket implementation of ParamSet (#15599)
# Objective

The `queue()` method is an optional trait method which is necessary for
deferred operations (such as command queues) to work properly in the
context of an observer.

This method was omitted from the proc_macro blanket implementation of
`ParamSet` for tuples; as a result, SystemParams with deferred
application (such as Commands) would not work in observers if they were
part of a ParamSet.

This appears to have been a simple omission, as `queue()` was already
implemented for the separate blanket implementation of `ParamSet` for
`Vec<T>`. In both cases, it is a simple pass-through to the component
SystemParams.

## Solution

Add the `queue()` method implementation to the `impl_param_set` proco
macro.

## Testing

Added a unit test which clearly demonstrates the issue. It fails before
the fix, and passes afterwards.

---
2024-10-02 19:46:50 +00:00
Matty
587a508ef9
Remove TransformCurve (#15598)
# Objective

It is somewhat unlikely we will actually be able to support
`TransformCurve` (introduced in #15434) after the `AnimationGraph`
evaluation order changes in the immediate future. This is because
correctly blending overlapping animation properties is nontrivial, and
`Transform` overlaps with all of its own fields. We could still
potentially create something like this in the future, but it's likely to
require significant design and implementation work. By way of contrast,
the single-property wrappers `TranslationCurve`, `ScaleCurve`, and
`RotationCurve` should work perfectly fine, since they are
non-overlapping.

In this version release, creating `TransformCurve` in userspace is also
quite easy if desired (see the deletions from this PR).

## Solution

Delete `TransformCurve`. 

## Migration Guide

There is no released version that contains this, but we should make sure
that `TransformCurve` is excluded from the release notes for #15434 if
we merge this pull request.
2024-10-02 19:46:38 +00:00
notmd
453c0167b2
Allow access a method handler (#15601)
# Objective

- I'm building a streaming plugin for `bevy_remote` and accessing to
builtin method will be very valuable

## Solution

- Add a method to allow access a handler by method name.

## Testing

- CI should pass
2024-10-02 19:45:18 +00:00
Christian Hughes
7c6057bc69
Enable EntityRef::get_by_id and friends to take multiple ids and get multiple pointers back (#15593)
# Objective

- Closes #15577 

## Solution

The following functions can now also take multiple component IDs and
return multiple pointers back:
- `EntityRef::get_by_id`
- `EntityMut::get_by_id`
- `EntityMut::into_borrow_by_id`
- `EntityMut::get_mut_by_id`
- `EntityMut::into_mut_by_id`
- `EntityWorldMut::get_by_id`
- `EntityWorldMut::into_borrow_by_id`
- `EntityWorldMut::get_mut_by_id`
- `EntityWorldMut::into_mut_by_id`

If you pass in X, you receive Y:
- give a single `ComponentId`, receive a single `Ptr`/`MutUntyped`
- give a `[ComponentId; N]` (array), receive a `[Ptr; N]`/`[MutUntyped;
N]`
- give a `&[ComponentId; N]` (array), receive a `[Ptr; N]`/`[MutUntyped;
N]`
- give a `&[ComponentId]` (slice), receive a
`Vec<Ptr>`/`Vec<MutUntyped>`
- give a `&HashSet<ComponentId>`, receive a `HashMap<ComponentId,
Ptr>`/`HashMap<ComponentId, MutUntyped>`

## Testing

- Added 4 new tests.

---

## Migration Guide

- The following functions now return an `Result<_,
EntityComponentError>` instead of a `Option<_>`: `EntityRef::get_by_id`,
`EntityMut::get_by_id`, `EntityMut::into_borrow_by_id`,
`EntityMut::get_mut_by_id`, `EntityMut::into_mut_by_id`,
`EntityWorldMut::get_by_id`, `EntityWorldMut::into_borrow_by_id`,
`EntityWorldMut::get_mut_by_id`, `EntityWorldMut::into_mut_by_id`
2024-10-02 19:02:20 +00:00
Tim
ca709ffb5a
Cleanup clearcoat example (#15594)
The components needed for `DirectionalLight` are added automatically
since #15554
`create_point_light` already existed and returns a `PointLight` with
these same settings
2024-10-02 16:15:27 +00:00
Robert Walter
59db6f9cca
add curve utilities to create curves interpolating/easing between two values (#14788)
# Objective

Citing @mweatherley 

> There is a lot of shortfall for simple cases— e.g., we should have
library functions for making a curve connecting two points, eased
versions of that, and so on.

## Solution

This PR implements

- a simple `Easing` trait which is implemented for all `impl Curve<f32>`
types. We can't really guarantee that these curves have unit interval
domain, which some people would probably expect, but it is documented
that this isn't the case for these types and we redirect to
`EasingCurve` which is used for that purpose
- an `EasingCurve` struct, which is used to interpolate between two
values `start` and `end` using a `impl Easing` curve where the curve
will be guaranteed to be reparametrized
- a `LinearCurve` which linearly interpolates between two values `start`
and `end`
- a `CubicBezierCurve` which interpolates between `start` and `end`
values using a `CubicSegment`
- a `StepCurve` which interpolates between `start` and `end` with an
step-function with `n` steps
- an `ElasticCurve` which interpolates between `start` and `end` with
spring like behavior where the elasticity of the spring is configurable
- some `FunctionCurve` easing curves for different popular functions
including: `quadratic_ease_in`, `quadratic_ease_out`, `smoothstep`,
`identity`

## Testing

- there are a few new tests for all of these in the main module

---------

Co-authored-by: eckz <567737+eckz@users.noreply.github.com>
Co-authored-by: Miles Silberling-Cook <NthTensor@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Matty <weatherleymatthew@gmail.com>
2024-10-02 14:29:05 +00:00
Dragoș Tiselice
ba7907cae7
Added visibility bitmask as an alternative SSAO method (#13454)
Early implementation. I still have to fix the documentation and consider
writing a small migration guide.

Questions left to answer:

* [x] should thickness be an overridable constant?
* [x] is there a better way to implement `Eq`/`Hash` for `SSAOMethod`?
* [x] do we want to keep the linear sampler for the depth texture?
* [x] is there a better way to separate the logic than preprocessor
macros?


![vbao](https://github.com/bevyengine/bevy/assets/4136413/2a8a0389-2add-4c2e-be37-e208e52dcd25)

## Migration guide

SSAO algorithm was changed from GTAO to VBAO (visibility bitmasks). A
new field, `constant_object_thickness`, was added to
`ScreenSpaceAmbientOcclusion`. `ScreenSpaceAmbientOcclusion` also lost
its `Eq` and `Hash` implementations.

---------

Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
2024-10-02 13:43:35 +00:00
Benjamin Brienen
c841dd92a1
Documentation for variadics (#15387)
# Objective

Relevant: #15208

## Solution

I went ahead and added the variadics documentation in all applicable
locations.

## Testing

- I built the documentation and inspected it to see whether the feature
is there.
2024-10-02 12:48:36 +00:00
Tim
461305b3d7
Revert "Have EntityCommands methods consume self for easier chaining" (#15523)
As discussed in #15521

- Partial revert of #14897, reverting the change to the methods to
consume `self`
- The `insert_if` method is kept

The migration guide of #14897 should be removed
Closes #15521

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
2024-10-02 12:47:26 +00:00
François Mockers
23b0dd6ffd
move ANDROID_APP to bevy_window (#15585)
# 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:🪟:android_activity`. Same for the `ANDROID_APP` static
2024-10-02 03:01:06 +00:00
Liam Gallagher
85dfd72631
Include errors along side successful components in BRP bevy/get method (#15516)
## Objective

I am using BRP for a web inspector. To get components from a entity is
first do a `bevy/list` on the specific entity and then use the result in
a `bevy/get` request. The problem with this is `bevy/list` returns all
components even if they aren't reflect-able (which is what I expect) but
when I then do a `bevy/get` request even if all bar one of the
components are reflect-able the request will fail.

## Solution

Update the `bevy/get` response to include a map of components like it
did for successful request and a map of errors. This means if one or
more components are not present on the entity or cannot be reflected it
will not fail the entire request.

I also only did `bevy/get` as I don't think any of the other methods
would benefit from this.

## Testing

I tested this with my inspector and with a http client and it worked as
expected.

---------

Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com>
2024-10-02 02:26:01 +00:00
Gino Valente
eaa37f3b45
bevy_reflect: Add DeserializeWithRegistry and SerializeWithRegistry (#8611)
# Objective

### The Problem

Currently, the reflection deserializers give little control to users for
how a type is deserialized. The most control a user can have is to
register `ReflectDeserialize`, which will use a type's `Deserialize`
implementation.

However, there are times when a type may require slightly more control.

For example, let's say we want to make Bevy's `Mesh` easier to
deserialize via reflection (assume `Mesh` actually implemented
`Reflect`). Since we want this to be extensible, we'll make it so users
can use their own types so long as they satisfy `Into<Mesh>`. The end
result should allow users to define a RON file like:

```rust
{
  "my_game::meshes::Sphere": (
    radius: 2.5
  )
}
```

### The Current Solution

Since we don't know the types ahead of time, we'll need to use
reflection. Luckily, we can access type information dynamically via the
type registry. Let's make a custom type data struct that users can
register on their types:

```rust
pub struct ReflectIntoMesh {
  // ...
}

impl<T: FromReflect + Into<Mesh>> FromType<T> for ReflectIntoMesh {
  fn from_type() -> Self {
    // ...
  }
}
```

Now we'll need a way to use this type data during deserialization.
Unfortunately, we can't use `Deserialize` since we need access to the
registry. This is where `DeserializeSeed` comes in handy:

```rust
pub struct MeshDeserializer<'a> {
  pub registry: &'a TypeRegistry
}

impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
  type Value = Mesh;

  fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
  where
    D: serde::Deserializer<'de>,
  {
    struct MeshVisitor<'a> {
      registry: &'a TypeRegistry
    }

    impl<'a, 'de> Visitor<'de> for MeshVisitor<'a> {
      fn expecting(&self, formatter: &mut Formatter) -> std::fmt::Result {
        write!(formatter, "map containing mesh information")
      }

      fn visit_map<A>(self, mut map: A) -> Result<Self::Value, serde:🇩🇪:Error> where A: MapAccess<'de> {
        // Parse the type name
        let type_name = map.next_key::<String>()?.unwrap();

        // Deserialize the value based on the type name
        let registration = self.registry
          .get_with_name(&type_name)
          .expect("should be registered");
        let value = map.next_value_seed(TypedReflectDeserializer {
          registration,
          registry: self.registry,
        })?;

        // Convert the deserialized value into a `Mesh`
        let into_mesh = registration.data::<ReflectIntoMesh>().unwrap();
        Ok(into_mesh.into(value))
      }
    }
  }
}
```

### The Problem with the Current Solution

The solution above works great when all we need to do is deserialize
`Mesh` directly. But now, we want to be able to deserialize a struct
like this:

```rust
struct Fireball {
  damage: f32,
  mesh: Mesh,
}
```

This might look simple enough and should theoretically be no problem for
the reflection deserializer to handle, but this is where our
`MeshDeserializer` solution starts to break down.

In order to use `MeshDeserializer`, we need to have access to the
registry. The reflection deserializers have access to that, but we have
no way of borrowing it for our own deserialization since they have no
way of knowing about `MeshDeserializer`.

This means we need to implement _another_ `DeserializeSeed`— this time
for `Fireball`!
And if we decided to put `Fireball` inside another type, well now we
need one for that type as well.

As you can see, this solution does not scale well and results in a lot
of unnecessary boilerplate for the user.

## Solution

> [!note]
> This PR originally only included the addition of
`DeserializeWithRegistry`. Since then, a corresponding
`SerializeWithRegistry` trait has also been added. The reasoning and
usage is pretty much the same as the former so I didn't bother to update
the full PR description.

Created the `DeserializeWithRegistry` trait and
`ReflectDeserializeWithRegistry` type data.

The `DeserializeWithRegistry` trait works like a standard `Deserialize`
but provides access to the registry. And by registering the
`ReflectDeserializeWithRegistry` type data, the reflection deserializers
will automatically use the `DeserializeWithRegistry` implementation,
just like it does for `Deserialize`.

All we need to do is make the following changes:

```diff
#[derive(Reflect)]
+ #[reflect(DeserializeWithRegistry)]
struct Mesh {
  // ...
}

- impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
-   type Value = Mesh;
-   fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
+ impl<'de> DeserializeWithRegistry<'de> for Mesh {
+   fn deserialize<D>(deserializer: D, registry: &TypeRegistry) -> Result<Self, D::Error>
    where
      D: serde::Deserializer<'de>,
    {
      // ...
    }
}
```

Now, any time the reflection deserializer comes across `Mesh`, it will
opt to use its `DeserializeWithRegistry` implementation. And this means
we no longer need to create a whole slew of `DeserializeSeed` types just
to deserialize `Mesh`.

### Why not a trait like `DeserializeSeed`?

While this would allow for anyone to define a deserializer for `Mesh`,
the problem is that it means __anyone can define a deserializer for
`Mesh`.__ This has the unfortunate consequence that users can never be
certain that their registration of `ReflectDeserializeSeed` is the one
that will actually be used.

We could consider adding something like that in the future, but I think
this PR's solution is much safer and follows the example set by
`ReflectDeserialize`.

### What if we made the `TypeRegistry` globally available?

This is one potential solution and has been discussed before (#6101).
However, that change is much more controversial and comes with its own
set of disadvantages (can't have multiple registries such as with
multiple worlds, likely some added performance cost with each access,
etc.).

### Followup Work

Once this PR is merged, we should consider merging `ReflectDeserialize`
into `DeserializeWithRegistry`. ~~There is already a blanket
implementation to make this transition generally pretty
straightforward.~~ The blanket implementations were removed for the sake
of this PR and will need to be re-added in the followup. I would propose
that we first mark `ReflectDeserialize` as deprecated, though, before we
outright remove it in a future release.

---

## Changelog

- Added the `DeserializeReflect` trait and `ReflectDeserializeReflect`
type data
- Added the `SerializeReflect` trait and `ReflectSerializeReflect` type
data
- Added `TypedReflectDeserializer::of` convenience constructor

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: aecsocket <43144841+aecsocket@users.noreply.github.com>
2024-10-02 01:54:32 +00:00
Viktor Gustavsson
f86ee32576
Add UI GhostNode (#15341)
# Objective

- Fixes #14826 
- For context, see #15238

## Solution
Add a `GhostNode` component to `bevy_ui` and update all the relevant
systems to use it to traverse for UI children.

- [x] `ghost_hierarchy` module
  - [x] Add `GhostNode`
- [x] Add `UiRootNodes` system param for iterating (ghost-aware) UI root
nodes
- [x] Add `UiChildren` system param for iterating (ghost-aware) UI
children
- [x] Update `layout::ui_layout_system`
  - [x] Use ghost-aware root nodes for camera updates
  - [x] Update and remove children in taffy
    - [x] Initial spawn
    - [x] Detect changes on nested UI children
- [x] Use ghost-aware children traversal in
`update_uinode_geometry_recursive`
- [x] Update the rest of the UI systems to use the ghost hierarchy
  - [x] `stack::ui_stack_system`
  - [x] `update::`
    - [x] `update_clipping_system`
    - [x] `update_target_camera_system`
  - [x] `accessibility::calc_name`

## Testing
- [x] Added a new example `ghost_nodes` that can be used as a testbed.
- [x] Added unit tests for _some_ of the traversal utilities in
`ghost_hierarchy`
- [x] Ensure this fulfills the needs for currently known use cases
  - [x] Reactivity libraries (test with `bevy_reactor`)
- [ ] Text spans (mentioned by koe [on
discord](https://discord.com/channels/691052431525675048/1285371432460881991/1285377442998915246))
  
---
## Performance
[See comment
below](https://github.com/bevyengine/bevy/pull/15341#issuecomment-2385456820)

## Migration guide
Any code that previously relied on `Parent`/`Children` to iterate UI
children may now want to use `bevy_ui::UiChildren` to ensure ghost nodes
are skipped, and their first descendant Nodes included.

UI root nodes may now be children of ghost nodes, which means
`Without<Parent>` might not query all root nodes. Use
`bevy_ui::UiRootNodes` where needed to iterate root nodes instead.

## Potential future work
- Benchmarking/optimizations of hierarchies containing lots of ghost
nodes
- Further exploration of UI hierarchies and markers for root nodes/leaf
nodes to create better ergonomics for things like `UiLayer` (world-space
ui)

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
2024-10-02 00:24:28 +00:00
UkoeHB
3df281ba7b
Refactor TextPipeline::update_buffer to accept an interator (#15581)
# Objective

- Prepare `TextPipeline` to work with multi-entity text blocks. See
https://github.com/bevyengine/bevy/discussions/15014

## Solution

- Refactor `TextPipeline::update_buffer` to accept an iterator instead
of slice. Adjust `update_buffer` implementation to only iterate spans
once instead of three times (which would require iterating a hierarchy
three times with multi-entity blocks).

## Testing

- Tested with `text_debug` example.
2024-10-01 23:44:59 +00:00
Liam Gallagher
d6cfafdfd4
Make SystemIdMarker reflect-able (#15556)
Make `SystemIdMarker` reflect-able.
2024-10-01 22:46:44 +00:00
Joona Aalto
22af24aacf
Migrate motion blur, TAA, SSAO, and SSR to required components (#15572)
# Objective

Again, a step forward in the migration to required components: a bunch
of camera rendering cormponents!

Note that this does not include the camera components themselves yet,
because the naming and API for `Camera` hasn't been fully decided yet.

## Solution

As per the [selected
proposals](https://hackmd.io/@bevy/required_components/%2FpiqD9GOdSFKZZGzzh3C7Uw):

- Deprecate `MotionBlurBundle` in favor of the `MotionBlur` component
- Deprecate `TemporalAntiAliasBundle` in favor of the
`TemporalAntiAliasing` component
- Deprecate `ScreenSpaceAmbientOcclusionBundle` in favor of the
`ScreenSpaceAmbientOcclusion` component
- Deprecate `ScreenSpaceReflectionsBundle` in favor of the
`ScreenSpaceReflections` component

---

## Migration Guide

`MotionBlurBundle`, `TemporalAntiAliasBundle`,
`ScreenSpaceAmbientOcclusionBundle`, and `ScreenSpaceReflectionsBundle`
have been deprecated in favor of the `MotionBlur`,
`TemporalAntiAliasing`, `ScreenSpaceAmbientOcclusion`, and
`ScreenSpaceReflections` components instead. Inserting them will now
also insert the other components required by them automatically.
2024-10-01 22:45:31 +00:00
Joona Aalto
ed151e756c
Migrate audio to required components (#15573)
# Objective

What's that? Another PR for the grand migration to required components?
This time, audio!

## Solution

Deprecate `AudioSourceBundle`, `AudioBundle`, and `PitchBundle`, as per
the [chosen
proposal](https://hackmd.io/@bevy/required_components/%2Fzxgp-zMMRUCdT7LY1ZDQwQ).

However, we cannot call the component `AudioSource`, because that's what
the stored asset is called. I deliberated on a few names, like
`AudioHandle`, or even just `Audio`, but landed on `AudioPlayer`, since
it's probably the most accurate and "nice" name for this. Open to
alternatives though.

---

## Migration Guide

Replace all insertions of `AudioSoucreBundle`, `AudioBundle`, and
`PitchBundle` with the `AudioPlayer` component. The other components
required by it will now be inserted automatically.

In cases where the generics cannot be inferred, you may need to specify
them explicitly. For example:

```rust
commands.spawn(AudioPlayer::<AudioSource>(asset_server.load("sounds/sick_beats.ogg")));
```
2024-10-01 22:43:29 +00:00
Tim
eb51b4c28e
Migrate scenes to required components (#15579)
# Objective

A step in the migration to required components: scenes!

## Solution

As per the [selected
proposal](https://hackmd.io/@bevy/required_components/%2FPJtNGVMMQhyM0zIvCJSkbA):
- Deprecate `SceneBundle` and `DynamicSceneBundle`.
- Add `SceneRoot` and `DynamicSceneRoot` components, which wrap a
`Handle<Scene>` and `Handle<DynamicScene>` respectively.

## Migration Guide
Asset handles for scenes and dynamic scenes must now be wrapped in the
`SceneRoot` and `DynamicSceneRoot` components. Raw handles as components
no longer spawn scenes.

Additionally, `SceneBundle` and `DynamicSceneBundle` have been
deprecated. Instead, use the scene components directly.

Previously:
```rust
let model_scene = asset_server.load(GltfAssetLabel::Scene(0).from_asset("model.gltf"));

commands.spawn(SceneBundle {
    scene: model_scene,
    transform: Transform::from_xyz(-4.0, 0.0, -3.0),
    ..default()
});
```
Now:
```rust
let model_scene = asset_server.load(GltfAssetLabel::Scene(0).from_asset("model.gltf"));

commands.spawn((
    SceneRoot(model_scene),
    Transform::from_xyz(-4.0, 0.0, -3.0),
));
```
2024-10-01 22:42:11 +00:00
UkoeHB
ead84e0e3d
Rename BreakLineOn to LineBreak (#15583)
# Objective

- Improve code quality in preparation for
https://github.com/bevyengine/bevy/discussions/15014

## Solution

- Rename BreakLineOn to LineBreak.

## Migration Guide

`BreakLineOn` was renamed to `LineBreak`, and paramters named
`linebreak_behavior` were renamed to `linebreak`.
2024-10-01 22:30:50 +00:00
Litttle_fish
e924df0e1a
Add features to switch NativeActivity and GameActivity usage (#12095)
# Objective

Add two features to switch bevy to use `NativeActivity` or
`GameActivity` on Android, use `GameActivity` by default.

Also close  #12058 and probably #12026 .

## Solution

Add two features to the corresponding crates so you can toggle it, like
what `winit` and `android-activity` crate did.

---

## Changelog

Removed default `NativeActivity` feature implementation for Android,
added two new features to enable `NativeActivity` and `GameActivity`,
and use `GameActivity` by default.

## Migration Guide

Because `cargo-apk` is not compatible with `GameActivity`,
building/running using `cargo apk build/run -p bevy_mobile_example` is
no longer possible.
Users should follow the new workflow described in document.

---------

Co-authored-by: François Mockers <francois.mockers@vleue.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: Rich Churcher <rich.churcher@gmail.com>
2024-10-01 22:23:48 +00:00
ickshonpe
f53af2846c
Update the UI texture slice pipeline to work with the retained render world changes (#15578)
# Objective

Update the UI's texture slice extraction to work with the changes from
the retained render world PR (#15320).
2024-10-01 22:02:02 +00:00
Joona Aalto
54006b107b
Migrate meshes and materials to required components (#15524)
# Objective

A big step in the migration to required components: meshes and
materials!

## Solution

As per the [selected
proposal](https://hackmd.io/@bevy/required_components/%2Fj9-PnF-2QKK0on1KQ29UWQ):

- Deprecate `MaterialMesh2dBundle`, `MaterialMeshBundle`, and
`PbrBundle`.
- Add `Mesh2d` and `Mesh3d` components, which wrap a `Handle<Mesh>`.
- Add `MeshMaterial2d<M: Material2d>` and `MeshMaterial3d<M: Material>`,
which wrap a `Handle<M>`.
- Meshes *without* a mesh material should be rendered with a default
material. The existence of a material is determined by
`HasMaterial2d`/`HasMaterial3d`, which is required by
`MeshMaterial2d`/`MeshMaterial3d`. This gets around problems with the
generics.

Previously:

```rust
commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});
```

Now:

```rust
commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));
```

If the mesh material is missing, previously nothing was rendered. Now,
it renders a white default `ColorMaterial` in 2D and a
`StandardMaterial` in 3D (this can be overridden). Below, only every
other entity has a material:

![Näyttökuva 2024-09-29
181746](https://github.com/user-attachments/assets/5c8be029-d2fe-4b8c-ae89-17a72ff82c9a)

![Näyttökuva 2024-09-29
181918](https://github.com/user-attachments/assets/58adbc55-5a1e-4c7d-a2c7-ed456227b909)

Why white? This is still open for discussion, but I think white makes
sense for a *default* material, while *invalid* asset handles pointing
to nothing should have something like a pink material to indicate that
something is broken (I don't handle that in this PR yet). This is kind
of a mix of Godot and Unity: Godot just renders a white material for
non-existent materials, while Unity renders nothing when no materials
exist, but renders pink for invalid materials. I can also change the
default material to pink if that is preferable though.

## Testing

I ran some 2D and 3D examples to test if anything changed visually. I
have not tested all examples or features yet however. If anyone wants to
test more extensively, it would be appreciated!

## Implementation Notes

- The relationship between `bevy_render` and `bevy_pbr` is weird here.
`bevy_render` needs `Mesh3d` for its own systems, but `bevy_pbr` has all
of the material logic, and `bevy_render` doesn't depend on it. I feel
like the two crates should be refactored in some way, but I think that's
out of scope for this PR.
- I didn't migrate meshlets to required components yet. That can
probably be done in a follow-up, as this is already a huge PR.
- It is becoming increasingly clear to me that we really, *really* want
to disallow raw asset handles as components. They caused me a *ton* of
headache here already, and it took me a long time to find every place
that queried for them or inserted them directly on entities, since there
were no compiler errors for it. If we don't remove the `Component`
derive, I expect raw asset handles to be a *huge* footgun for users as
we transition to wrapper components, especially as handles as components
have been the norm so far. I personally consider this to be a blocker
for 0.15: we need to migrate to wrapper components for asset handles
everywhere, and remove the `Component` derive. Also see
https://github.com/bevyengine/bevy/issues/14124.

---

## Migration Guide

Asset handles for meshes and mesh materials must now be wrapped in the
`Mesh2d` and `MeshMaterial2d` or `Mesh3d` and `MeshMaterial3d`
components for 2D and 3D respectively. Raw handles as components no
longer render meshes.

Additionally, `MaterialMesh2dBundle`, `MaterialMeshBundle`, and
`PbrBundle` have been deprecated. Instead, use the mesh and material
components directly.

Previously:

```rust
commands.spawn(MaterialMesh2dBundle {
    mesh: meshes.add(Circle::new(100.0)).into(),
    material: materials.add(Color::srgb(7.5, 0.0, 7.5)),
    transform: Transform::from_translation(Vec3::new(-200., 0., 0.)),
    ..default()
});
```

Now:

```rust
commands.spawn((
    Mesh2d(meshes.add(Circle::new(100.0))),
    MeshMaterial2d(materials.add(Color::srgb(7.5, 0.0, 7.5))),
    Transform::from_translation(Vec3::new(-200., 0., 0.)),
));
```

If the mesh material is missing, a white default material is now used.
Previously, nothing was rendered if the material was missing.

The `WithMesh2d` and `WithMesh3d` query filter type aliases have also
been removed. Simply use `With<Mesh2d>` or `With<Mesh3d>`.

---------

Co-authored-by: Tim Blackbird <justthecooldude@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
2024-10-01 21:33:17 +00:00
Joona Aalto
0fe17b8b30
Migrate fog volumes to required components (#15568)
# Objective

Another part of the migration to required components: fog volumes!

## Solution

Deprecate `FogVolumeBundle` and make `FogVolume` require `Transform` and
`Visibility`, as per the [chosen
proposal](https://hackmd.io/@bevy/required_components/%2FcO7JPSAQR5G0J_j5wNwtOQ).

---

## Migration Guide

Replace all insertions of `FogVolumeBundle` with the `Visibility`
component. The other components required by it will now be inserted
automatically.
2024-10-01 19:51:22 +00:00
Josh Robson Chase
40e88dceff
Change ReflectMapEntities to operate on components before insertion (#15422)
Previous PR https://github.com/bevyengine/bevy/pull/14549 was closed in
error and couldn't be reopened since I had updated the branch
😿

# Objective

Fixes #14465

## Solution

`ReflectMapEntities` now works similarly to `MapEntities` in that it
works on the reflected value itself rather than the component in the
world after insertion. This makes it so that observers see the remapped
entities on insertion rather than the entity IDs from the scene.

`ReflectMapEntities` now works for both components and resources, so we
only need the one.

## Testing

* New unit test for `Observer`s + `DynamicScene`s
* New unit test for `Observer`s + `Scene`s
* Open to suggestions for other tests!

---

## Migration Guide

- Consumers of `ReflectMapEntities` will need to call `map_entities` on
values prior to inserting them into the world.
- Implementors of `MapEntities` will need to remove the `mappings`
method, which is no longer needed for `ReflectMapEntities` and has been
removed from the trait.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>
2024-10-01 18:34:09 +00:00
vero
6465e3bd9f
Fix Mesh allocator bug and reduce Mesh data copies by two (#15566)
# Objective

- First step towards #15558

## Solution

- Rename `get_vertex_buffer_data` to `create_packed_vertex_buffer_data`
to make it clear that it is not "free" and actually allocates
- Compute length analytically for preallocation instead of creating the
buffer to get its length and immediately discard it
- Use existing vertex attribute size calculation method to reduce code
duplication
- Fix a bug where mesh index data was being replaced by unnecessarily
newly created mesh vertex data in some cases
- Overall reduces mesh copies by two. We still have plenty to go, but
these were the easy ones.

## Testing

- I ran 3d_scene, lighting, and many_cubes, they look fine.
- Benchmarks would be nice, but this is very obviously a win in perf and
correctness.

---

## Migration Guide

- `Mesh::create_packed_vertex_buffer_data` has been renamed
`Mesh::create_packed_vertex_buffer_data` to reflect the fact that it
copies data and allocates.

## Showcase

- look mom, less copies
2024-10-01 17:15:57 +00:00
aecsocket
1df8238e8d
bevy_asset: Improve NestedLoader API (#15509)
# Objective

The `NestedLoader` API as it stands right now is somewhat lacking:

- It consists of several types `NestedLoader`, `UntypedNestedLoader`,
`DirectNestedLoader`, and `UntypedDirectNestedLoader`, where a typestate
pattern on `NestedLoader` would be make it more obvious what it does,
and allow centralising the documentation
- The term "untyped" in the asset loader code is overloaded. It can mean
either:
- we have literally no idea what the type of this asset will be when we
load it (I dub this "unknown type")
- we know what type of asset it will be, but we don't know it statically
- we only have a TypeId (I dub this "dynamic type" / "erased")
- There is no way to get an `UntypedHandle` (erased) given a `TypeId`

## Solution

Changes `NestedLoader` into a type-state pattern, adding two type
params:
- `T` determines the typing
- `StaticTyped`, the default, where you pass in `A` statically into `fn
load<A>() -> ..`
- `DynamicTyped`, where you give a `TypeId`, giving you a
`UntypedHandle`
- `UnknownTyped`, where you have literally no idea what type of asset
you're loading, giving you a `Handle<LoadedUntypedAsset>`
- `M` determines the "mode" (bikeshedding TBD, I couldn't come up with a
better name)
- `Deferred`, the default, won't load the asset when you call `load`,
but it does give you a `Handle` to it (this is nice since it can be a
sync fn)
- `Immediate` will load the asset as soon as you call it, and give you
access to it, but you must be in an async context to call it

Changes some naming of internals in `AssetServer` to fit the new
definitions of "dynamic type" and "unknown type". Note that I didn't do
a full pass over this code to keep the diff small. That can probably be
done in a new PR - I think the definiton I laid out of unknown type vs.
erased makes it pretty clear where each one applies.

<details>
<summary>Old issue</summary>

The only real problem I have with this PR is the requirement to pass in
`type_name` (from `core::any::type_name`) into Erased. Users might not
have that type name, only the ID, and it just seems sort of weird to
*have* to give an asset type name. However, the reason we need it is
because of this:
```rs
    pub(crate) fn get_or_create_path_handle_erased(
        &mut self,
        path: AssetPath<'static>,
        type_id: TypeId,
        type_name: &str,
        loading_mode: HandleLoadingMode,
        meta_transform: Option<MetaTransform>,
    ) -> (UntypedHandle, bool) {
        let result = self.get_or_create_path_handle_internal(
            path,
            Some(type_id),
            loading_mode,
            meta_transform,
        );
        // it is ok to unwrap because TypeId was specified above
        unwrap_with_context(result, type_name).unwrap()
    }

pub(crate) fn unwrap_with_context<T>(
    result: Result<T, GetOrCreateHandleInternalError>,
    type_name: &str,
) -> Option<T> {
    match result {
        Ok(value) => Some(value),
        Err(GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified) => None,
        Err(GetOrCreateHandleInternalError::MissingHandleProviderError(_)) => {
            panic!("Cannot allocate an Asset Handle of type '{type_name}' because the asset type has not been initialized. \
                    Make sure you have called app.init_asset::<{type_name}>()")
        }
    }
}
```
This `unwrap_with_context` is literally the only reason we need the
`type_name`. Potentially, this can be turned into an `impl
Into<Option<&str>>`, and output a different error message if the type
name is missing. Since if we are loading an asset where we only know the
type ID, by definition we can't output that error message, since we
don't have the type name. I'm open to suggestions on this.

</details>

## Testing

Not sure how to test this, since I kept most of the actual NestedLoader
logic the same. The only new API is loading an `UntypedHandle` when in
the `DynamicTyped, Immediate` state.

## Migration Guide

Code which uses `bevy_asset`'s `LoadContext::loader` / `NestedLoader`
will see some naming changes:
- `untyped` is replaced by `with_unknown_type`
- `with_asset_type` is replaced by `with_static_type`
- `with_asset_type_id` is replaced by `with_dynamic_type`
- `direct` is replaced by `immediate` (the opposite of "immediate" is
"deferred")
2024-10-01 14:14:04 +00:00
m-edlund
c323db02e0
Add sub_camera_view, enabling sheared projection (#15537)
# Objective

- This PR fixes #12488

## Solution

- This PR adds a new property to `Camera` that emulates the
functionality of the
[setViewOffset()](https://threejs.org/docs/#api/en/cameras/PerspectiveCamera.setViewOffset)
API in three.js.
- When set, the perspective and orthographic projections will restrict
the visible area of the camera to a part of the view frustum defined by
`offset` and `size`.

## Testing

- In the new `camera_sub_view` example, a fixed, moving and control sub
view is created for both perspective and orthographic projection
- Run the example with `cargo run --example camera_sub_view`
- The code can be tested by adding a `SubCameraView` to a camera

---

## Showcase


![image](https://github.com/user-attachments/assets/75ac45fc-d75d-4664-8ef6-ff7865297c25)

- Left Half: Perspective Projection
- Right Half: Orthographic Projection
- Small boxes in order:
  - Sub view of the left half of the full image
- Sub view moving from the top left to the bottom right of the full
image
  - Sub view of the full image (acting as a control)
- Large box: No sub view

<details>
  <summary>Shortened camera setup of `camera_sub_view` example</summary>

```rust
    // Main perspective Camera
    commands.spawn(Camera3dBundle {
        transform,
        ..default()
    });

    // Perspective camera left half
    commands.spawn(Camera3dBundle {
        camera: Camera {
            sub_camera_view: Some(SubCameraView {
                // Set the sub view camera to the left half of the full image
                full_size: uvec2(500, 500),
                offset: ivec2(0, 0),
                size: uvec2(250, 500),
            }),
            order: 1,
            ..default()
        },
        transform,
        ..default()
    });

    // Perspective camera moving
    commands.spawn((
        Camera3dBundle {
            camera: Camera {
                sub_camera_view: Some(SubCameraView {
                    // Set the sub view camera to a fifth of the full view and
                    // move it in another system
                    full_size: uvec2(500, 500),
                    offset: ivec2(0, 0),
                    size: uvec2(100, 100),
                }),
                order: 2,
                ..default()
            },
            transform,
            ..default()
        },
        MovingCameraMarker,
    ));

    // Perspective camera control
    commands.spawn(Camera3dBundle {
        camera: Camera {
            sub_camera_view: Some(SubCameraView {
                // Set the sub view to the full image, to ensure that it matches
                // the projection without sub view
                full_size: uvec2(450, 450),
                offset: ivec2(0, 0),
                size: uvec2(450, 450),
            }),
            order: 3,
            ..default()
        },
        transform,
        ..default()
    });

    // Main orthographic camera
    commands.spawn(Camera3dBundle {
        projection: OrthographicProjection {
          ...
        }
        .into(),
        camera: Camera {
            order: 4,
            ..default()
        },
        transform,
        ..default()
    });

    // Orthographic camera left half
    commands.spawn(Camera3dBundle {
        projection: OrthographicProjection {
          ...
        }
        .into(),
        camera: Camera {
            sub_camera_view: Some(SubCameraView {
                // Set the sub view camera to the left half of the full image
                full_size: uvec2(500, 500),
                offset: ivec2(0, 0),
                size: uvec2(250, 500),
            }),
            order: 5,
            ..default()
        },
        transform,
        ..default()
    });

    // Orthographic camera moving
    commands.spawn((
        Camera3dBundle {
            projection: OrthographicProjection {
              ...
            }
            .into(),
            camera: Camera {
                sub_camera_view: Some(SubCameraView {
                    // Set the sub view camera to a fifth of the full view and
                    // move it in another system
                    full_size: uvec2(500, 500),
                    offset: ivec2(0, 0),
                    size: uvec2(100, 100),
                }),
                order: 6,
                ..default()
            },
            transform,
            ..default()
        },
        MovingCameraMarker,
    ));

    // Orthographic camera control
    commands.spawn(Camera3dBundle {
        projection: OrthographicProjection {
          ...
        }
        .into(),
        camera: Camera {
            sub_camera_view: Some(SubCameraView {
                // Set the sub view to the full image, to ensure that it matches
                // the projection without sub view
                full_size: uvec2(450, 450),
                offset: ivec2(0, 0),
                size: uvec2(450, 450),
            }),
            order: 7,
            ..default()
        },
        transform,
        ..default()
    });
```

</details>
2024-10-01 14:11:24 +00:00
Ensar Sarajčić
956d9ccbb1
Add directory related functions to AndroidAssetReader (#11495)
# Objective

- Fixes #9968

## Solution

- Uses
[open_dir](https://docs.rs/ndk/latest/ndk/asset/struct.AssetManager.html#method.open_dir)
to read directories and collects child list, since it can't be shared
across threads.
- For `is_directory`, uses result of
[open](https://docs.rs/ndk/latest/ndk/asset/struct.AssetManager.html#method.open),
which will fail for directories. I tried using the result of `open_dir`
for this, but it was successful for files too, which made loading
folders return empty lists, since `open_dir` was successful and treated
all files as empty directories.
- Ignoring `meta` files was copied from filesystem implementation

---

## Changelog

- Fixed: Android's AssetReader implementation now supports
read_directory and is_directory.

## Notes

I noticed late that there was the #9968 issue (I only noticed #9591), so
I have also missed that a PR was already open (#9969). Feel free to copy
over the fixes from this one over there.
The only difference I notice between these 2, is that I have used `open`
instead of `open_dir` for `is_directory` implementation. I have tried
with `open_dir` too, but unfortunately that didn't work. I tested this
on an actual device, using the mobile example, by making some minor
changes:

```rust
#[derive(Resource)]
struct FolderAssets(Handle<LoadedFolder>);

// the `bevy_main` proc_macro generates the required boilerplate for iOS and Android
#[bevy_main]
fn main() {
    // ...
    .add_systems(Startup, (setup_scene, load_music_files))
    .add_systems(
        Update,
        // Removed the handle_lifetime since AudioBundle is added later
        (touch_camera, button_handler, setup_music),
    );
   // ...
}

fn load_music_files(asset_server: Res<AssetServer>, mut commands: Commands) {
    let sounds = asset_server.load_folder("sounds");
    commands.insert_resource(FolderAssets(sounds));
}

fn setup_music(
    mut commands: Commands,
    folders: Res<Assets<LoadedFolder>>,
    mut loaded_event: EventReader<AssetEvent<LoadedFolder>>,
) {
    for event in loaded_event.read() {
        if let AssetEvent::LoadedWithDependencies { id } = event {
            if let Some(folder) = folders.get(*id) {
                warn!("Folder items: {:?}", folder.handles);
                if let Some(source) = folder.handles.first() {
                    commands.spawn(AudioBundle {
                        source: source.clone().typed::<AudioSource>(),
                        settings: PlaybackSettings::LOOP,
                    });
                }
            }
        }
    }
}
```

---------

Co-authored-by: Kanabenki <lucien.menassol@gmail.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
2024-10-01 12:23:24 +00:00
Ludwig DUBOS
f08f07785b
Replace AsyncSeek trait by AsyncSeekForward for Reader to address #12880 (#14194)
# Objective

The primary motivation behind this PR is to (partially?) address the
limitations imposed by the recently added `AsyncSeek` trait bound
discussed in issue #12880. While the `AsyncSeek` trait add some
flexibility to the reader, it inadvertently restricts the ability to
write asset readers that can truly stream bytes, particularly in
scenarios like HTTP requests where backward seeking is not supported. It
is also challenging in contexts where assets are stored in compressed
formats or require other kinds of transformations.

The logic behind this change is that currently, with `AsyncSeek`, an
asset Reader based on streamed data will either 1) fail silently, 2)
return an error, or 3) use a buffer to satisfy the trait constraint. I
believe that being able to advance in the file without having to "read"
it is a good thing. The only issue here is the ability to seek backward.
It is highly likely that in this context, we only need to seek forward
in the file because we would have already read an entry table upstream
and just want to access one or more resources further in the file. I
understand that in some cases, this may not be applicable, but I think
it is more beneficial not to constrain `Reader`s that want to stream
than to allow "Assets" to read files in a completely arbitrary order.

## Solution

Replace the current `AsyncSeek` trait with `AsyncSeekForward` on asset
`Reader`

## Changelog

- Introduced a new custom trait, `AsyncSeekForward`, for the asset
Reader.
- Replaced the current `AsyncSeek` trait with `AsyncSeekForward` for all
asset `Reader` implementations.

## Migration Guide

Replace all instances of `AsyncSeek` with `AsyncSeekForward` in your
asset reader implementations.
2024-10-01 03:33:45 +00:00
Joona Aalto
de888a373d
Migrate lights to required components (#15554)
# Objective

Another step in the migration to required components: lights!

Note that this does not include `EnvironmentMapLight` or reflection
probes yet, because their API hasn't been fully chosen yet.

## Solution

As per the [selected
proposals](https://hackmd.io/@bevy/required_components/%2FLLnzwz9XTxiD7i2jiUXkJg):

- Deprecate `PointLightBundle` in favor of the `PointLight` component
- Deprecate `SpotLightBundle` in favor of the `PointLight` component
- Deprecate `DirectionalLightBundle` in favor of the `DirectionalLight`
component

## Testing

I ran some examples with lights.

---

## Migration Guide

`PointLightBundle`, `SpotLightBundle`, and `DirectionalLightBundle` have
been deprecated. Use the `PointLight`, `SpotLight`, and
`DirectionalLight` components instead. Adding them will now insert the
other components required by them automatically.
2024-10-01 03:20:43 +00:00
Andreas
383c2e5bd7
15540 Make World::flush_commands private (#15553)
# Objective

Fixes #15540 

End-users risk using `World::flush_commands` instead of `World::flush`,
which panics if any queued commands are `spawn`. Hiding
`World::flush_commands` would help avoid calling a potentially panicky
function, and helps alleviate end-user API confusion.

## Solution

This PR updates the function visibility to crate-level, like
`World::flush_entities`, hiding it from the end-user while still making
it accessible for the tests that are currently set up.

## Testing

The change was tested by executing the available tests for `bevy_ecs`.
From what I've gathered, `World::flush_commands` is not used in any
other bevy crate. If further testing is recommended, please inform me!

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
2024-09-30 22:07:09 +00:00
Kristoffer Søholm
73af2b7d29
Cleanup unneeded lifetimes in bevy_asset (#15546)
# Objective

Fixes #15541

A bunch of lifetimes were added during the Assets V2 rework, but after
moving to async traits in #12550 they can be elided. That PR mentions
that this might be the case, but apparently it wasn't followed up on at
the time.

~~I ended up grepping for `<'a` and finding a similar case in
`bevy_reflect` which I also fixed.~~ (edit: that one was needed
apparently)

Note that elided lifetimes are unstable in `impl Trait`. If that gets
stabilized then we can elide even more.

## Solution

Remove the extra lifetimes.

## Testing

Everything still compiles. If I have messed something up there is a
small risk that some user code stops compiling, but all the examples
still work at least.

---

## Migration Guide

The traits `AssetLoader`, `AssetSaver` and `Process` traits from
`bevy_asset` now use elided lifetimes. If you implement these then
remove the named lifetime.
2024-09-30 21:54:59 +00:00
Liam Gallagher
10068f4a26
Add content-type header to BRP HTTP responses (#15552)
This makes HTTP clients like httpie format the response as JSON rather
than text for a much nicer experience testing things.
2024-09-30 21:23:55 +00:00
Matty
429987ebf8
Curve-based animation (#15434)
# Objective

This PR extends and reworks the material from #15282 by allowing
arbitrary curves to be used by the animation system to animate arbitrary
properties. The goals of this work are to:
- Allow far greater flexibility in how animations are allowed to be
defined in order to be used with `bevy_animation`.
- Delegate responsibility over keyframe interpolation to `bevy_math` and
the `Curve` libraries and reduce reliance on keyframes in animation
definitions generally.
- Move away from allowing the glTF spec to completely define animations
on a mechanical level.

## Solution

### Overview

At a high level, curves have been incorporated into the animation system
using the `AnimationCurve` trait (closely related to what was
`Keyframes`). From the top down:

1. In `animate_targets`, animations are driven by `VariableCurve`, which
is now a thin wrapper around a `Box<dyn AnimationCurve>`.
2. `AnimationCurve` is something built out of a `Curve`, and it tells
the animation system how to use the curve's output to actually mutate
component properties. The trait looks like this:
```rust
/// A low-level trait that provides control over how curves are actually applied to entities
/// by the animation system.
///
/// Typically, this will not need to be implemented manually, since it is automatically
/// implemented by [`AnimatableCurve`] and other curves used by the animation system
/// (e.g. those that animate parts of transforms or morph weights). However, this can be
/// implemented manually when `AnimatableCurve` is not sufficiently expressive.
///
/// In many respects, this behaves like a type-erased form of [`Curve`], where the output
/// type of the curve is remembered only in the components that are mutated in the
/// implementation of [`apply`].
///
/// [`apply`]: AnimationCurve::apply
pub trait AnimationCurve: Reflect + Debug + Send + Sync {
    /// Returns a boxed clone of this value.
    fn clone_value(&self) -> Box<dyn AnimationCurve>;

    /// The range of times for which this animation is defined.
    fn domain(&self) -> Interval;

    /// Write the value of sampling this curve at time `t` into `transform` or `entity`,
    /// as appropriate, interpolating between the existing value and the sampled value
    /// using the given `weight`.
    fn apply<'a>(
        &self,
        t: f32,
        transform: Option<Mut<'a, Transform>>,
        entity: EntityMutExcept<'a, (Transform, AnimationPlayer, Handle<AnimationGraph>)>,
        weight: f32,
    ) -> Result<(), AnimationEvaluationError>;
}
```
3. The conversion process from a `Curve` to an `AnimationCurve` involves
using wrappers which communicate the intent to animate a particular
property. For example, here is `TranslationCurve`, which wraps a
`Curve<Vec3>` and uses it to animate `Transform::translation`:
```rust
/// This type allows a curve valued in `Vec3` to become an [`AnimationCurve`] that animates
/// the translation component of a transform.
pub struct TranslationCurve<C>(pub C);
```

### Animatable Properties

The `AnimatableProperty` trait survives in the transition, and it can be
used to allow curves to animate arbitrary component properties. The
updated documentation for `AnimatableProperty` explains this process:
<details>
  <summary>Expand AnimatableProperty example</summary

An `AnimatableProperty` is a value on a component that Bevy can animate.

You can implement this trait on a unit struct in order to support
animating
custom components other than transforms and morph weights. Use that type
in
conjunction with `AnimatableCurve` (and perhaps
`AnimatableKeyframeCurve`
to define the animation itself). For example, in order to animate font
size of a
text section from 24 pt. to 80 pt., you might use:

```rust
#[derive(Reflect)]
struct FontSizeProperty;

impl AnimatableProperty for FontSizeProperty {
    type Component = Text;
    type Property = f32;
    fn get_mut(component: &mut Self::Component) -> Option<&mut Self::Property> {
        Some(&mut component.sections.get_mut(0)?.style.font_size)
    }
}
```

You can then create an `AnimationClip` to animate this property like so:

```rust
let mut animation_clip = AnimationClip::default();
animation_clip.add_curve_to_target(
    animation_target_id,
    AnimatableKeyframeCurve::new(
        [
            (0.0, 24.0),
            (1.0, 80.0),
        ]
    )
    .map(AnimatableCurve::<FontSizeProperty, _>::from_curve)
    .expect("Failed to create font size curve")
);
```

Here, the use of `AnimatableKeyframeCurve` creates a curve out of the
given keyframe time-value
pairs, using the `Animatable` implementation of `f32` to interpolate
between them. The
invocation of `AnimatableCurve::from_curve` with `FontSizeProperty`
indicates that the `f32`
output from that curve is to be used to animate the font size of a
`Text` component (as
configured above).


</details>

### glTF Loading

glTF animations are now loaded into `Curve` types of various kinds,
depending on what is being animated and what interpolation mode is being
used. Those types get wrapped into and converted into `Box<dyn
AnimationCurve>` and shoved inside of a `VariableCurve` just like
everybody else.

### Morph Weights

There is an `IterableCurve` abstraction which allows sampling these from
a contiguous buffer without allocating. Its only reason for existing is
that Rust disallows you from naming function types, otherwise we would
just use `Curve` with an iterator output type. (The iterator involves
`Map`, and the name of the function type would have to be able to be
named, but it is not.)

A `WeightsCurve` adaptor turns an `IterableCurve` into an
`AnimationCurve`, so it behaves like everything else in that regard.

## Testing

Tested by running existing animation examples. Interpolation logic has
had additional tests added within the `Curve` API to replace the tests
in `bevy_animation`. Some kinds of out-of-bounds errors have become
impossible.

Performance testing on `many_foxes` (`animate_targets`) suggests that
performance is very similar to the existing implementation. Here are a
couple trace histograms across different runs (yellow is this branch,
red is main).
<img width="669" alt="Screenshot 2024-09-27 at 9 41 50 AM"
src="https://github.com/user-attachments/assets/5ba4e9ac-3aea-452e-aaf8-1492acc2d7fc">
<img width="673" alt="Screenshot 2024-09-27 at 9 45 18 AM"
src="https://github.com/user-attachments/assets/8982538b-04cf-46b5-97b2-164c6bc8162e">

---

## Migration Guide

Most user code that does not directly deal with `AnimationClip` and
`VariableCurve` will not need to be changed. On the other hand,
`VariableCurve` has been completely overhauled. If you were previously
defining animation curves in code using keyframes, you will need to
migrate that code to use curve constructors instead. For example, a
rotation animation defined using keyframes and added to an animation
clip like this:
```rust
animation_clip.add_curve_to_target(
    animation_target_id,
    VariableCurve {
        keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
        keyframes: Keyframes::Rotation(vec![
            Quat::IDENTITY,
            Quat::from_axis_angle(Vec3::Y, PI / 2.),
            Quat::from_axis_angle(Vec3::Y, PI / 2. * 2.),
            Quat::from_axis_angle(Vec3::Y, PI / 2. * 3.),
            Quat::IDENTITY,
        ]),
        interpolation: Interpolation::Linear,
    },
);
```

would now be added like this:
```rust
animation_clip.add_curve_to_target(
    animation_target_id,
    AnimatableKeyframeCurve::new([0.0, 1.0, 2.0, 3.0, 4.0].into_iter().zip([
        Quat::IDENTITY,
        Quat::from_axis_angle(Vec3::Y, PI / 2.),
        Quat::from_axis_angle(Vec3::Y, PI / 2. * 2.),
        Quat::from_axis_angle(Vec3::Y, PI / 2. * 3.),
        Quat::IDENTITY,
    ]))
    .map(RotationCurve)
    .expect("Failed to build rotation curve"),
);
```

Note that the interface of `AnimationClip::add_curve_to_target` has also
changed (as this example shows, if subtly), and now takes its curve
input as an `impl AnimationCurve`. If you need to add a `VariableCurve`
directly, a new method `add_variable_curve_to_target` accommodates that
(and serves as a one-to-one migration in this regard).

### For reviewers

The diff is pretty big, and the structure of some of the changes might
not be super-obvious:
- `keyframes.rs` became `animation_curves.rs`, and `AnimationCurve` is
based heavily on `Keyframes`, with the adaptors also largely following
suite.
- The Curve API adaptor structs were moved from `bevy_math::curve::mod`
into their own module `adaptors`. There are no functional changes to how
these adaptors work; this is just to make room for the specialized
reflection implementations since `mod.rs` was getting kind of cramped.
- The new module `gltf_curves` holds the additional curve constructions
that are needed by the glTF loader. Note that the loader uses a mix of
these and off-the-shelf `bevy_math` curve stuff.
- `animatable.rs` no longer holds logic related to keyframe
interpolation, which is now delegated to the existing abstractions in
`bevy_math::curve::cores`.

---------

Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Co-authored-by: aecsocket <43144841+aecsocket@users.noreply.github.com>
2024-09-30 19:56:55 +00:00
Joona Aalto
f3e8ae03cd
Runtime required components (#15458)
# Objective

Fixes #15367.

Currently, required components can only be defined through the `require`
macro attribute. While this should be used in most cases, there are also
several instances where you may want to define requirements at runtime,
commonly in plugins.

Example use cases:

- Require components only if the relevant optional plugins are enabled.
For example, a `SleepTimer` component (for physics) is only relevant if
the `SleepPlugin` is enabled.
- Third party crates can define their own requirements for first party
types. For example, "each `Handle<Mesh>` should require my custom
rendering data components". This also gets around the orphan rule.
- Generic plugins that add marker components based on the existence of
other components, like a generic `ColliderPlugin<C: AnyCollider>` that
wants to add a `ColliderMarker` component for all types of colliders.
- This is currently relevant for the retained render world in #15320.
The `ExtractComponentPlugin<C>` should add `SyncToRenderWorld` to all
components that should be extracted. This is currently done with
observers, which is more expensive than required components, and causes
archetype moves.
- Replace some built-in components with custom versions. For example, if
`GlobalTransform` required `Transform` through `TransformPlugin`, but we
wanted to use a `CustomTransform` type, we could replace
`TransformPlugin` with our own plugin. (This specific example isn't
good, but there are likely better use cases where this may be useful)

See #15367 for more in-depth reasoning.

## Solution

Add `register_required_components::<T, R>` and
`register_required_components_with::<T, R>` methods for `Default` and
custom constructors respectively. These methods exist on `App` and
`World`.

```rust
struct BirdPlugin;

impl Plugin for BirdPlugin {
    fn plugin(app: &mut App) {
        // Make `Bird` require `Wings` with a `Default` constructor.
        app.register_required_components::<Bird, Wings>();

        // Make `Wings` require `FlapSpeed` with a custom constructor.
        // Fun fact: Some hummingbirds can flutter their wings 80 times per second!
        app.register_required_components_with::<Wings, FlapSpeed>(|| FlapSpeed::from_duration(1.0 / 80.0));
    }
}
```

The custom constructor is a function pointer to match the `require` API,
though it could take a raw value too.

Requirement inheritance works similarly as with the `require` attribute.
If `Bird` required `FlapSpeed` directly, it would take precedence over
indirectly requiring it through `Wings`. The same logic applies to all
levels of the inheritance tree.

Note that registering the same component requirement more than once will
panic, similarly to trying to add multiple component hooks of the same
type to the same component. This avoids constructor conflicts and
confusing ordering issues.

### Implementation

Runtime requirements have two additional challenges in comparison to the
`require` attribute.

1. The `require` attribute uses recursion and macros with clever
ordering to populate hash maps of required components for each component
type. The expected semantics are that "more specific" requirements
override ones deeper in the inheritance tree. However, at runtime, there
is no representation of how "specific" each requirement is.
2. If you first register the requirement `X -> Y`, and later register `Y
-> Z`, then `X` should also indirectly require `Z`. However, `Y` itself
doesn't know that it is required by `X`, so it's not aware that it
should update the list of required components for `X`.

My solutions to these problems are:

1. Store the depth in the inheritance tree for each entry of a given
component's `RequiredComponents`. This is used to determine how
"specific" each requirement is. For `require`-based registration, these
depths are computed as part of the recursion.
2. Store and maintain a `required_by` list in each component's
`ComponentInfo`, next to `required_components`. For `require`-based
registration, these are also added after each registration, as part of
the recursion.

When calling `register_required_components`, it works as follows:

1. Get the required components of `Foo`, and check that `Bar` isn't
already a *direct* requirement.
3. Register `Bar` as a required component for `Foo`, and add `Foo` to
the `required_by` list for `Bar`.
4. Find and register all indirect requirements inherited from `Bar`,
adding `Foo` to the `required_by` list for each component.
5. Iterate through components that require `Foo`, registering the new
inherited requires for them as indirect requirements.

The runtime registration is likely slightly more expensive than the
`require` version, but it is a one-time cost, and quite negligible in
practice, unless projects have hundreds or thousands of runtime
requirements. I have not benchmarked this however.

This does also add a small amount of extra cost to the `require`
attribute for updating `required_by` lists, but I expect it to be very
minor.

## Testing

I added some tests that are copies of the `require` versions, as well as
some tests that are more specific to the runtime implementation. I might
add a few more tests though.

## Discussion

- Is `register_required_components` a good name? Originally I went for
`register_component_requirement` to be consistent with
`register_component_hooks`, but the general feature is often referred to
as "required components", which is why I changed it to
`register_required_components`.
- Should we *not* panic for duplicate requirements? If so, should they
just be ignored, or should the latest registration overwrite earlier
ones?
- If we do want to panic for duplicate, conflicting registrations,
should we at least not panic if the registrations are *exactly* the
same, i.e. same component and same constructor? The current
implementation panics for all duplicate direct registrations regardless
of the constructor.

## Next Steps

- Allow `register_required_components` to take a `Bundle` instead of a
single required component.
    - I could also try to do it in this PR if that would be preferable.
- Not directly related, but archetype invariants?
2024-09-30 19:20:16 +00:00