Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reflection: Add distinction between ignoring fields for purposes of scene deserialization and external visibility #5245

Closed
makspll opened this issue Jul 7, 2022 · 3 comments
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible

Comments

@makspll
Copy link
Contributor

makspll commented Jul 7, 2022

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

This stems from a discussion on discord.

Currently the reflection system is closely coupled with scene serialisation. This unavoidably causes the propagation of #[reflect(ignore)]'s throughout the codebase on fields which:

  1. are part of the public interface
  2. do implement Reflect
  3. are not intended to be serialized to the scene
    An example of this can be seen in bevy::render::Camera :
#[derive(Component, Debug, Reflect, Clone)]
#[reflect(Component)]
pub struct Camera {
    /// If set, this camera will render to the given [`Viewport`] rectangle within the configured [`RenderTarget`].
    pub viewport: Option<Viewport>,
    /// Cameras with a lower priority will be rendered before cameras with a higher priority.
    pub priority: isize,
    /// If this is set to true, this camera will be rendered to its specified [`RenderTarget`]. If false, this
    /// camera will not be rendered.
    pub is_active: bool,
    /// The method used to calculate this camera's depth. This will be used for projections and visibility.
    pub depth_calculation: DepthCalculation,
    /// Computed values for this camera, such as the projection matrix and the render target size.
    #[reflect(ignore)]
    pub computed: ComputedCameraValues,
    /// The "target" that this camera will render to.
    #[reflect(ignore)]
    pub target: RenderTarget,
}

in this example RenderTarget should not be hidden away from for example script users or the editor. There is tension between goals 1. and 3.

What solution would you like?

Separate the notions of ignoring fields from reflection and just serialization,
perhaps an attribute like: #[reflect(dont_scene_serialize)]
would help make the intent clearer and solve the tension between two different use cases.

What alternative(s) have you considered?

  • Making serialisation to scene opt-in, by perhaps introducing another trait.
@makspll makspll added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 7, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Jul 7, 2022

Currently the reflection system is closely coupled with scene serialisation.

The issue is that while they’re conceptually coupled, the derive and the serialization don’t really communicate with each other directly. We have use the registry to drive serde stuff (and adding another trait won't help much without the registry).

So it's going to be difficult to make specific fields reflectable but not serializable.

One solution might be to add a map in the registry that maps field name/index to a Boolean flag indicating whether the field can be serialized or not (HashSet works too). This adds more complexity to the registry but allows this to be more easily configured by both the derive and manually.

It would probably be easier to just mark the entire struct (i.e. the field's type) with an attribute that prevents serialization. Then we don't need a map or need to worry about "name vs index," but it's debatable whether that would truly be better (personally, I think it makes more sense for the original type to determine if it's serializable or not) and match most use cases.

@makspll
Copy link
Contributor Author

makspll commented Jul 7, 2022

Disabling serialisation for an entire type would be fairly rigid, and you never know if a type might need to be serialised in another context.

And yeah, I don't really see another solution short of keeping track of indices/field names.

@MrGVSV MrGVSV added A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Jul 7, 2022
bors bot pushed a commit that referenced this issue Sep 19, 2022
… automatic serialization (#5250)

# Objective

- To address problems outlined in #5245

## Solution

- Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field.

---

## Changelog
- Adds: 
  - `bevy_reflect::serde::type_data` module
  - `ReflectSerializableWithData` trait for retrieving `SerializationData` which is implemented via macros
  - `SerializationData` structure for describing which fields are to be/not to be ignored
  - the `skip_serialization` flag for `#[reflect(...)]`
 - Removes:
   - ability to ignore Enum variants in serialization, since that didn't work anyway   
 

## Migration Guide
- Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect.
- Remove ignore/skip attributes from enum variants as these won't do anything anymore
bors bot pushed a commit that referenced this issue Sep 19, 2022
… automatic serialization (#5250)

# Objective

- To address problems outlined in #5245

## Solution

- Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field.

---

## Changelog
- Adds: 
  - `bevy_reflect::serde::type_data` module
  - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types
  - the `skip_serialization` flag for `#[reflect(...)]`
 - Removes:
   - ability to ignore Enum variants in serialization, since that didn't work anyway   
 

## Migration Guide
- Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect.
- Remove ignore/skip attributes from enum variants as these won't do anything anymore
@MrGVSV MrGVSV moved this to Open in Reflection Oct 15, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Oct 15, 2022

Closing as this should be resolved by #5250. @makspll feel free to re-open if you feel it wasn't fully addressed!

@MrGVSV MrGVSV closed this as completed Oct 15, 2022
@MrGVSV MrGVSV moved this from Open to Done in Reflection Oct 15, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 19, 2022
… automatic serialization (bevyengine#5250)

# Objective

- To address problems outlined in bevyengine#5245

## Solution

- Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field.

---

## Changelog
- Adds: 
  - `bevy_reflect::serde::type_data` module
  - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types
  - the `skip_serialization` flag for `#[reflect(...)]`
 - Removes:
   - ability to ignore Enum variants in serialization, since that didn't work anyway   
 

## Migration Guide
- Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect.
- Remove ignore/skip attributes from enum variants as these won't do anything anymore
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
… automatic serialization (bevyengine#5250)

# Objective

- To address problems outlined in bevyengine#5245

## Solution

- Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field.

---

## Changelog
- Adds: 
  - `bevy_reflect::serde::type_data` module
  - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types
  - the `skip_serialization` flag for `#[reflect(...)]`
 - Removes:
   - ability to ignore Enum variants in serialization, since that didn't work anyway   
 

## Migration Guide
- Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect.
- Remove ignore/skip attributes from enum variants as these won't do anything anymore
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
… automatic serialization (bevyengine#5250)

# Objective

- To address problems outlined in bevyengine#5245

## Solution

- Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field.

---

## Changelog
- Adds: 
  - `bevy_reflect::serde::type_data` module
  - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types
  - the `skip_serialization` flag for `#[reflect(...)]`
 - Removes:
   - ability to ignore Enum variants in serialization, since that didn't work anyway   
 

## Migration Guide
- Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect.
- Remove ignore/skip attributes from enum variants as these won't do anything anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible
Projects
Status: Done
Development

No branches or pull requests

2 participants