Skip to content

[Merged by Bors] - Add apply_or_insert functions to reflected component and resources - #5201

Closed
Shatur wants to merge 3 commits into
bevyengine:mainfrom
gardum-game:apply-or-insert
Closed

[Merged by Bors] - Add apply_or_insert functions to reflected component and resources#5201
Shatur wants to merge 3 commits into
bevyengine:mainfrom
gardum-game:apply-or-insert

Conversation

@Shatur

@Shatur Shatur commented Jul 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Objective

ReflectResource and ReflectComponent will panic on apply method if there is no such component. It's not very ergonomic. And not very good for performance since I need to check if such component exists first.

Solution

  • Add ReflectComponent::apply_or_insert and ReflectResource::apply_or_insert functions.
  • Rename ReflectComponent::add into ReflectComponent::insert for consistency.

Changelog

Added

  • ReflectResource::apply_or_insert and ReflectComponent::apply_on_insert.

Changed

  • Rename ReflectComponent::add into ReflectComponent::insert for consistency.
  • Use ReflectComponent::apply_on_insert in DynamicScene instead of manual checking.

Migration Guide

  • Rename ReflectComponent::add into ReflectComponent::insert.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jul 4, 2022
alice-i-cecile
alice-i-cecile reviewed Jul 4, 2022
View reviewed changes
Comment thread crates/bevy_ecs/src/reflect.rs Resolved
alice-i-cecile
alice-i-cecile reviewed Jul 4, 2022
View reviewed changes
Comment thread crates/bevy_ecs/src/reflect.rs Resolved
alice-i-cecile
alice-i-cecile approved these changes Jul 4, 2022
View reviewed changes

@alice-i-cecile alice-i-cecile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM once the doc comment changes are in.

@Shatur Shatur mentioned this pull request Jul 4, 2022
Closed
@Shatur

Shatur commented Jul 4, 2022

Copy link
Copy Markdown
Contributor Author

While working on it I decided to open an issue with naming suggestions: #5202

@Shatur Shatur changed the title Add apply_or_insert_* functions to reflected component and resources Add apply_or_insert functions to reflected component and resources Jul 6, 2022
@Shatur

Shatur commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

@alice-i-cecile rebased after #5219.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 7, 2022
jakobhellermann
jakobhellermann approved these changes Jul 10, 2022
View reviewed changes

@jakobhellermann jakobhellermann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks good to me.
Long term I wonder whether we can get rid of ReflectComponent and ReflectResource and built these APIs on top of the raw untyped methods + ReflectFromPtr, but that can be considered separately.

@alice-i-cecile

alice-i-cecile commented Jul 11, 2022

Copy link
Copy Markdown
Member

bors r+

bors Bot pushed a commit that referenced this pull request Jul 11, 2022
@Shatur
Add apply_or_insert functions to reflected component and resources (#…
1da6720 
…5201)

# Objective

`ReflectResource` and `ReflectComponent` will panic on `apply` method if there is no such component. It's not very ergonomic. And not very good for performance since I need to check if such component exists first.

## Solution

* Add `ReflectComponent::apply_or_insert` and `ReflectResource::apply_or_insert` functions.
* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.

---

## Changelog

### Added

* `ReflectResource::apply_or_insert` and `ReflectComponent::apply_on_insert`.

### Changed

* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.
* Use `ReflectComponent::apply_on_insert` in `DynamicScene` instead of manual checking.

## Migration Guide

* Rename `ReflectComponent::add` into `ReflectComponent::insert`.
@bors

bors Bot commented Jul 11, 2022

Copy link
Copy Markdown

Pull request successfully merged into main.

Build succeeded:

@bors bors Bot changed the title Add apply_or_insert functions to reflected component and resources [Merged by Bors] - Add apply_or_insert functions to reflected component and resources Jul 11, 2022
@bors bors Bot closed this Jul 11, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@Shatur @inodentry
Add apply_or_insert functions to reflected component and resources (b…
514c908 
…evyengine#5201)

# Objective

`ReflectResource` and `ReflectComponent` will panic on `apply` method if there is no such component. It's not very ergonomic. And not very good for performance since I need to check if such component exists first.

## Solution

* Add `ReflectComponent::apply_or_insert` and `ReflectResource::apply_or_insert` functions.
* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.

---

## Changelog

### Added

* `ReflectResource::apply_or_insert` and `ReflectComponent::apply_on_insert`.

### Changed

* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.
* Use `ReflectComponent::apply_on_insert` in `DynamicScene` instead of manual checking.

## Migration Guide

* Rename `ReflectComponent::add` into `ReflectComponent::insert`.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@Shatur @james7132
Add apply_or_insert functions to reflected component and resources (b…
e8e1406 
…evyengine#5201)

# Objective

`ReflectResource` and `ReflectComponent` will panic on `apply` method if there is no such component. It's not very ergonomic. And not very good for performance since I need to check if such component exists first.

## Solution

* Add `ReflectComponent::apply_or_insert` and `ReflectResource::apply_or_insert` functions.
* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.

---

## Changelog

### Added

* `ReflectResource::apply_or_insert` and `ReflectComponent::apply_on_insert`.

### Changed

* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.
* Use `ReflectComponent::apply_on_insert` in `DynamicScene` instead of manual checking.

## Migration Guide

* Rename `ReflectComponent::add` into `ReflectComponent::insert`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@Shatur @ItsDoot
Add apply_or_insert functions to reflected component and resources (b…
2ba0e86 
…evyengine#5201)

# Objective

`ReflectResource` and `ReflectComponent` will panic on `apply` method if there is no such component. It's not very ergonomic. And not very good for performance since I need to check if such component exists first.

## Solution

* Add `ReflectComponent::apply_or_insert` and `ReflectResource::apply_or_insert` functions.
* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.

---

## Changelog

### Added

* `ReflectResource::apply_or_insert` and `ReflectComponent::apply_on_insert`.

### Changed

* Rename `ReflectComponent::add` into `ReflectComponent::insert` for consistency.
* Use `ReflectComponent::apply_on_insert` in `DynamicScene` instead of manual checking.

## Migration Guide

* Rename `ReflectComponent::add` into `ReflectComponent::insert`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes
@PROMETHIA-27 PROMETHIA-27 Awaiting requested review from PROMETHIA-27
@MrGVSV MrGVSV Awaiting requested review from MrGVSV
@jakobhellermann jakobhellermann Awaiting requested review from jakobhellermann

Assignees

No one assigned

Labels

A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Shatur @alice-i-cecile @jakobhellermann @PROMETHIA-27