hankjordan
Update Bevy to 0.14
I haven't fixed a couple of tests because I'm unsure of why they are failing in the way they are. The test test_json expects that the entities get ids 0,1,2,3,4 but the ids seem to start from u32::MAX + 1 for some reason. I also saw that when I ran the examples straight from master, the entity ids put into the json file were huge. This is despite the ids themselves starting from 0 before serialization. So I am unsure if this behavior is caused by the update of Bevy itself or something that differs in my environment.
Closes #37
I spent some time investigating this issue, which resulted in this minimally-reproducible example:
#[test]
fn test_entity_bits() {
let test_entity = Entity::from_raw(0);
let mut actual_output_buffer = Vec::new();
let formatter = serde_json::ser::CompactFormatter;
let mut serializer = serde_json::Serializer::with_formatter(&mut actual_output_buffer, formatter);
test_entity.serialize(&mut serializer).unwrap();
let output_string = String::from_utf8(actual_output_buffer).unwrap();
let output_u64 = u64::from_str_radix(output_string.as_str(), 10).unwrap();
assert_eq!(test_entity.to_bits(), output_u64);
let reconstructed_entity = Entity::from_bits(output_u64);
assert_eq!(test_entity, reconstructed_entity);
}
It should be noted that this example completely bypasses the code in the rest of this repository, which demonstrates that bevy_save is not responsible for this change in behavior. Serde is providing the exact same u64 as Entity::to_bits, and they both agree in the values starting at 4294967296 per the OP.
The code for Entity is as follows:
<...snip...>
#[repr(C, align(8))]
pub struct Entity {
// Do not reorder the fields here. The ordering is explicitly used by repr(C)
// to make this struct equivalent to a u64.
#[cfg(target_endian = "little")]
index: u32,
generation: NonZeroU32,
#[cfg(target_endian = "big")]
index: u32,
}
I don't have a good enough grasp of low-level memory layouts to describe exactly why the bit representation changed, but the git blame leads here, so I suspect that this change introduced the change in behavior, perhaps by altering a flag in the upper address space: https://github.com/bevyengine/bevy/commit/b257fffef81a1c3a33c867c39dd53fb10bc6fd3b
Given the consensus between bevy_ecs and serde, I feel confident that this is expected behavior, and the values observed are correct as ordered by the upstream. My recommendation is to accept the behavior change and change the test data instead.
Ok, I finished the investigation. That commit was indeed the culprit, particularly this change, and other ones like it throughout the rest of the file: https://github.com/bevyengine/bevy/commit/b257fffef81a1c3a33c867c39dd53fb10bc6fd3b#diff-64745065a7cefb843c5c5a881c561330ee9d06d3f1f36c28321788b97e64d88fR249-R254
In short, entity generations now begin at one instead of zero, for optimization reasons explained in the commit message, and since the generation is encoded in the higher-magnitude bits of the u64 representation, this results in an extra u32::MAX being added to all such encoded entities, which is the observed behavior here.
This reinforces the conclusion that the behavior is nominal, and the tests should be altered to pass.
@NEON725 Great digging!
I guess the lack of automated testing in this repo made it slip through the tracks?
In my view it would be bad practice to depend on the underlying representation for the test as it is now, because the representation might change again. I think it's better to return the entity ids directly from the function and check against those instead of hardcoded numbers. I'll try to push a fix for that either tonight or tomorrow unless someone protests.
I don't agree that we're lacking good testing here. There was a breaking change in behavior, and the tests brought it to our attention.
I agree with this view that we shouldn't hard-code IDs that are beyond our control. Since these are given by the upstream, it should be included as part of the precondition. Should be some simple string interpolation of the expected data using the to_bits function to provide the values dynamically.
Once those changes are made, I think this PR is ready to un-draft and pull in @hankjordan for final approval.
I don't agree that we're lacking good testing here.
I just meant that there doesn't seem to be any nightly/push pipeline. Then the change of behavior would have been caught much earlier.
Maybe my solution to the byte slice test was a bit overcomplicated...
Realized that code in docs were failing as well. One place I can't quite figure out.
---- src/lib.rs - (line 45) stdout ----
error[E0425]: cannot find value `WORKSPACE` in this scope
--> src/lib.rs:46:1
|
3 | WORKSPACE.KEY
| ^^^^^^^^^ not found in this scope
|
help: consider importing this constant
|
2 + use bevy_save::WORKSPACE;
|
@perry-blueberry The line number is deceptive. It's caused by the include_str! directive at the top of src/lib.rs, which is inserting the entire doc inline, which makes the compiler see a much larger file.
Actual error is here: https://github.com/perry-blueberry/bevy_save/blob/update-to-bevy-0.14/README.md?plain=1#L38
This is because it's interpreting the markdown syntax as a doctest. All code blocks not annotated with a language are assumed to be testable Rust code, per https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#documentation-tests
Adding the ignore keyword in the README.md fixes the error.
I can't figure out how to propose a change here in the PR, so here's the patch. Unzip and use git am in your local repository.
I can't figure out how to propose a change here in the PR, so here's the patch. Unzip and use
git amin your local repository.
I applied the patch. All tests seem to be working now.
@hankjordan Any ETA on getting this merged? Counting the above PR and my local projects, there are now multiple downstream consumers for these changes.
Apologies for letting this rot for so long. I had intended to do a complete overhaul following the 0.13 release but that never materialized. @perry-blueberry I really do appreciate the contributions here. I am preparing a release for bevy 0.14 and a follow-up for bevy 0.15. Thanks, all.