-
-
Notifications
You must be signed in to change notification settings - Fork 506
Upgrade to rkyv 0.8 #1502
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
base: main
Are you sure you want to change the base?
Upgrade to rkyv 0.8 #1502
Conversation
|
||
let archived = rkyv::check_archived_root::<$ty<f32>>(&bytes[..]).unwrap(); | ||
let archived = rkyv::access::<<$ty<f32> as rkyv::Archive>::Archived, rkyv::rancor::Error>(&bytes) | ||
.unwrap(); | ||
// Compare archived and non-archived | ||
assert_eq!(archived, &value); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug check underneath is adding a constraint on Archived
to be Debug
, which was not explicit before, I think that's what we'll want to do like what I did for Matrix
through archive_bounds(<S as Archive>::Archived: core::fmt::Debug)
, but it feels like it's adding potentially unnecessary constraints.
Could a manual implementation could implement Debug only for compatible types ?
Also, the debug format test is failing currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going through ~as = Matrix<Archived::T>
and leveraging Portable
is probably the best path forward, but I'm not sure how exactly.
compare(PartialEq, PartialOrd), | ||
derive(Debug), | ||
archive_bounds( | ||
<S as Archive>::Archived: core::fmt::Debug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is a bit constraining ? This is only needed for automated derive (needed for tests)
See https://github.com/dimforge/nalgebra/pull/1502/files#r2025182401
@@ -161,16 +157,18 @@ pub type MatrixCross<T, R1, C1, R2, C2> = | |||
#[cfg_attr( | |||
feature = "rkyv-serialize-no-std", | |||
derive(Archive, rkyv::Serialize, rkyv::Deserialize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to derive rkyv::Portable
? I'm not sure what the implications are.
I think this would allow us to avoid a specific ArchivedMatrix
being created, which is likely what we want, as was the case with as Matrix
.
/// A translation. | ||
#[repr(C)] | ||
#[cfg_attr( | ||
feature = "rkyv-serialize-no-std", | ||
derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize), | ||
archive( | ||
as = "Translation<T::Archived, D>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as
being removed is a UX downgrade because we can't access underlying functions from original type on rkyv archived type.
@@ -5,21 +5,21 @@ use na::{ | |||
Rotation3, Similarity3, SimilarityMatrix2, SimilarityMatrix3, Translation2, Translation3, | |||
}; | |||
use rand; | |||
use rkyv::{Deserialize, Infallible}; | |||
|
|||
macro_rules! test_rkyv_same_type( | |||
($($test: ident, $ty: ident);* $(;)*) => {$( | |||
#[test] | |||
fn $test() { | |||
let value: $ty<f32> = rand::random(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a new test on i32 or something different than f32, as currently this would fail due to missing partialEq from arraystorage.
Helpful resources
rkyv
wasmerio/wasmer#5142helpful commits from rkyv
Current state
deny(missing_docs)
fails because of Add documentation for struct fields rkyv/rkyv#603Difficulties
rkyv(as = )
forMatrix
.i32
is notPortable
. See WIP migration to 0.8 rkyv/rkyv_contrib#4 for a somewhat similar situation.ArrayStorage
, so it must be something else in play.as =
makes it impossible to use rkyv(derive(...)), so we have no other choice to implement those manually. It's a bit boiler-plate-y because of_le
rkyv types.Previous analysis for `as`
In current nalgebra, we're using:
I believe we'll want to keep using that
as =
; I'm not sure what's the advantage of that exactly, I guess it limits the number of types, to avoid ending up with too many archived types, as well as avoid type juggling. (so compilation and runtime performance) ; ❓ any guess confirmation is welcome 😄I ended up with this migration, mostly reading (and forcing) through errors:
PartialEq
between arraystorage is currently not generic