Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - Enable Constructing ReflectComponent/Resource #6257

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Enable Constructing ReflectComponent/Resource #6257

wants to merge 2 commits into from

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Oct 14, 2022

Objective

Solution

  • Create a constructor for creating ReflectComponent and ReflectResource

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

Added

  • Created constructors for ReflectComponent and ReflectResource, allowing for advanced scripting use-cases.

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 14, 2022
alice-i-cecile
alice-i-cecile reviewed Oct 14, 2022
View reviewed changes
crates/bevy_ecs/src/reflect.rs
@@ -27,7 +27,36 @@ pub struct ReflectComponent {
copy: fn(&World, &mut World, Entity, Entity),
}

/// The raw functions needed to make up a [`ReflectComponent`].
///
/// This is used when creating custom implementations of [`ReflectComponent`] with
Copy link
Member

Choose a reason for hiding this comment

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

We should link to / explain the simpler way here. Ditto for the resource equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We should probably also include it in the docs for the constructors (e.g. ReflectComponent::new) as well, and indicate that this is almost certainly not needed unless you know what you're doing.

alice-i-cecile
alice-i-cecile requested changes Oct 14, 2022
View reviewed changes
Copy link
Member

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

Choose a reason for hiding this comment

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

I like the general approach. I think the docs need a bit of work. If you can add a bit more advice about how this might be used in practice (even if not a full example), I think this will be significantly less mysterious to users.

I'm not entirely sure on the best place for that, so use your judgement :)

MrGVSV
MrGVSV reviewed Oct 14, 2022
View reviewed changes
examples/ecs/system_closure.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect.rs
/// This is used when creating custom implementations of [`ReflectComponent`] with
/// [`ReflectComponent::new()`].
pub struct ReflectComponentFns {
pub insert: fn(&mut World, Entity, &dyn Reflect),
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a doc comment for each of these (same for ReflectResourceFns)? I imagine the people who do need this could benefit from knowing what they actually do.

crates/bevy_ecs/src/reflect.rs
///
/// This is used when creating custom implementations of [`ReflectComponent`] with
/// [`ReflectComponent::new()`].
pub struct ReflectComponentFns {
Copy link
Member

Choose a reason for hiding this comment

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

Should these structs have their own ReflectComponentFns::new<T: Component>() -> Self functions to fill in the entire struct, allowing individual modification if needed? Or does it not make sense to only override some but not all function pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually a good idea. I'm not 100% sure that there will be a use for overriding only some options, but it seems like it could happen.

@zicklag zicklag force-pushed the construct-reflect-component-resource branch 3 times, most recently from 86dddef to be21e55 Compare October 14, 2022 16:51
@zicklag
Copy link
Member Author

zicklag commented Oct 14, 2022

Pushed a bunch of docs, and extra helper methods like you guys suggested.

MrGVSV
MrGVSV reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for updating the docs. Just a few more comments from me.

crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
@zicklag zicklag force-pushed the construct-reflect-component-resource branch from be21e55 to 7c46bf7 Compare October 14, 2022 19:02
@zicklag
Copy link
Member Author

zicklag commented Oct 14, 2022

Pushed another update.

@zicklag zicklag force-pushed the construct-reflect-component-resource branch from 7c46bf7 to fb37d6b Compare October 14, 2022 19:04
MrGVSV
MrGVSV approved these changes Oct 14, 2022
View reviewed changes
crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
@zicklag @MrGVSV
Fix Typos in Doc Comments
d9f6046 
Co-authored-by: Gino Valente <[email protected]>
alice-i-cecile
alice-i-cecile approved these changes Oct 15, 2022
View reviewed changes
Copy link
Member

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

Choose a reason for hiding this comment

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

Great docs, and very well made. I trust your judgement that this is in fact very useful :p

@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 Oct 15, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 17, 2022
@zicklag
Enable Constructing ReflectComponent/Resource (#6257)
f9c56b3 
# Objective

- Fixes #6206

## Solution

- Create a constructor for creating `ReflectComponent` and `ReflectResource`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

### Added

- Created constructors for `ReflectComponent` and `ReflectResource`, allowing for advanced scripting use-cases.
@bors
Copy link
Contributor

bors bot commented Oct 17, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Enable Constructing ReflectComponent/Resource [Merged by Bors] - Enable Constructing ReflectComponent/Resource Oct 17, 2022
@bors bors bot closed this Oct 17, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
@zicklag @james7132
Enable Constructing ReflectComponent/Resource (bevyengine#6257)
240839e 
# Objective

- Fixes bevyengine#6206

## Solution

- Create a constructor for creating `ReflectComponent` and `ReflectResource`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

### Added

- Created constructors for `ReflectComponent` and `ReflectResource`, allowing for advanced scripting use-cases.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@zicklag @james7132
Enable Constructing ReflectComponent/Resource (bevyengine#6257)
0966207 
# Objective

- Fixes bevyengine#6206

## Solution

- Create a constructor for creating `ReflectComponent` and `ReflectResource`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

### Added

- Created constructors for `ReflectComponent` and `ReflectResource`, allowing for advanced scripting use-cases.
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@zicklag @Pietrek14
Enable Constructing ReflectComponent/Resource (bevyengine#6257)
208be5c 
# Objective

- Fixes bevyengine#6206

## Solution

- Create a constructor for creating `ReflectComponent` and `ReflectResource`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

### Added

- Created constructors for `ReflectComponent` and `ReflectResource`, allowing for advanced scripting use-cases.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@zicklag @ItsDoot
Enable Constructing ReflectComponent/Resource (bevyengine#6257)
e633624 
# Objective

- Fixes bevyengine#6206

## Solution

- Create a constructor for creating `ReflectComponent` and `ReflectResource`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

### Added

- Created constructors for `ReflectComponent` and `ReflectResource`, allowing for advanced scripting use-cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@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
Reflection
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow Custom Implementations of ReflectComponent
4 participants
@zicklag @alice-i-cecile @MrGVSV @mockersf