Fix: deserialize DynamicEnum using index

[mrchantey]

Objective

• Addresses DynamicScene - Malformed enum when reserializing #12462
• When we serialize an enum, deserialize it, then reserialize it, the correct variant should be selected.

Solution

• Change dynamic_enum.set_variant to dynamic_enum.set_variant_with_index in EnumVisitor

[MrGVSV]

I think I found the fix if you want to incorporate it in this PR:

The problem is here:

bevy/crates/bevy_reflect/src/serde/de.rs

Line 757 in 0e6a171

dynamic_enum.set_variant(variant_info.name(), value);

We need to change it to:

let variant_name = variant_info.name();
let variant_index = self
  .enum_info
  .index_of(variant_name)
  .expect("variant should exist");
dynamic_enum.set_variant_with_index(variant_index, variant_name, value);

[alice-i-cecile]

Great start! Can you add a matching test in bevy_reflect to check the serialization there? Presumably that's where the actual bug is: the scene machinery is likely redundant.

[mrchantey]

Awesome I added the bevy_reflect test, should I remove the bevy_scene test?

[MrGVSV]

Awesome I added the bevy_reflect test, should I remove the bevy_scene test?

Yeah I think you can remove it. It would be nice to have a re-serialization test but I think the reflection test sorta makes it redundant. Maybe if there's certain behavior we want to verify re-serializes correctly, we add the test then.

But yeah as for now, I don't think it's super necessary.

[mrchantey]

Okay, refactored enum_should_reserialize into should_reserialize for MyStruct, now getting a new error:

---- serde::de::tests::should_reserialize stdout ----
thread 'serde::de::tests::should_reserialize' panicked at crates\bevy_reflect\src\serde\de.rs:1449:74:
called `Result::unwrap()` on an `Err` value: Message("Unexpected missing field `renamed` in `CustomDeserialize`")

Did we catch another bug? commenting out #[serde(rename = "renamed")] fixes the new test.

btw i also deduplicated the three identical MyStruct instantiations instead of copy-pasting a new one, let me know if that should be reverted.

[MrGVSV]

Okay, refactored enum_should_reserialize into should_reserialize for MyStruct, now getting a new error:

---- serde::de::tests::should_reserialize stdout ----

thread 'serde::de::tests::should_reserialize' panicked at crates\bevy_reflect\src\serde\de.rs:1449:74:

called `Result::unwrap()` on an `Err` value: Message("Unexpected missing field `renamed` in `CustomDeserialize`")

Did we catch another bug? commenting out #[serde(rename = "renamed")] fixes the new test.

No I think this is expected behavior. We're serializing the Rust name but saying we expect the custom name (for a user the fix would be to register ReflectSerialize as well, otherwise they're declaring a one-way format).

Maybe let's try changing rename to alias? That should allow both to be used.

[mrchantey]

Great yep using alias did the trick, I can confirm the new test fails without set_variant_with_index.

[alice-i-cecile]

Great stuff: thanks for the contribution!