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

bevy_reflect: #[reflect(skip_serializing)] doesn't work on enum variant fields #6721

Open
MrGVSV opened this issue Nov 21, 2022 · 0 comments · May be fixed by #6767
Open

bevy_reflect: #[reflect(skip_serializing)] doesn't work on enum variant fields #6721

MrGVSV opened this issue Nov 21, 2022 · 0 comments · May be fixed by #6767
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior

Comments

@MrGVSV
Copy link
Member

MrGVSV commented Nov 21, 2022

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

Reflected enums do not support SerializationData. This is because SerializationData works for a single set of fields but enums can have multiple (one set per variant).

This means that we #[reflect(skip_serializing)] is ignored for a variant's fields. For example, this does nothing:

#[derive(Reflect)]
enum Foo {
  A {
    #[reflect(skip_serializing)]
    value: i32,
  }
}

What solution would you like?

Ideally we should enable this behavior on enums. They should register and store the ignored set per variant. To do so, we probably need to change SerializationData into an enum with one variant for structs and another for enums.

Alternatively, we could rethink SerializationData and make its data available on the TypeInfo itself (or on the individual NamedField and UnnamedField info structs). The reason we might not want to do this is doing so begins to (1) add bloat to those structs and (2) tightly couple reflect-related logic with reflection itself. It might be okay for just this attribute, but we may want to be cautious of how much we make "intrinsic" to reflection.

What alternative(s) have you considered?

We could potentially just disallow this behavior. That's definitely not ideal but we could easily add a compile error if this attribute is placed on a variant's field.

@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Nov 21, 2022
bors bot pushed a commit that referenced this issue Nov 23, 2022
…#6722)

# Objective 

Fixes #6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see #6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
Shatur pushed a commit to projectharmonia/bevy that referenced this issue Nov 25, 2022
…bevyengine#6722) [skip ci]

Fixes bevyengine#6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see bevyengine#6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

- Fix bug where deserializing unit structs would fail for non-self-describing formats
cart pushed a commit that referenced this issue Nov 30, 2022
…#6722)

# Objective 

Fixes #6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see #6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…bevyengine#6722)

# Objective 

Fixes bevyengine#6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see bevyengine#6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
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-Bug An unexpected or incorrect behavior
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

1 participant