UE5.4 support, struct names, binrw - #147
Conversation
localcc
left a comment
There was a problem hiding this comment.
Regarding the hints, I will look if the binrw crate can be trivially modified (potentially upstreamed) for access of the property stack
| @@ -0,0 +1,116 @@ | |||
| use std::{fmt::Display, str::FromStr}; | |||
|
|
|||
| use chrono::{DateTime, Utc}; | |||
There was a problem hiding this comment.
should chrono be an optional feature enabled by default?
There was a problem hiding this comment.
Good idea, I will try it. I think chrono is only used for serde -> Display/FromStr -> chrono, so it might make sense to tie it to the serde feature.
| #[brw(args(format))] TOptional<NumberFormattingOptions>, | ||
| FString, | ||
| ), | ||
| // #[brw(magic = 5i8)] AsPercent(FormatArgumentValue, TOptional<NumberFormattingOptions>, FString), |
There was a problem hiding this comment.
what's the reason for those being commented out?
There was a problem hiding this comment.
I think the comments are correct from my read of the code, but I have not verified them because the existing test fixtures don't have examples of these.
| // #[brw(magic = -1i8)] | ||
| // #[br(pre_assert(!format.culture_invariant_stability))] | ||
| // Empty(), | ||
| #[brw(magic = -1i8)] | ||
| // #[br(pre_assert(format.culture_invariant_stability))] |
There was a problem hiding this comment.
these comments can be removed, FTextHistoryNone handles that now.
| // #[brw(magic = -1i8)] | |
| // #[br(pre_assert(!format.culture_invariant_stability))] | |
| // Empty(), | |
| #[brw(magic = -1i8)] | |
| // #[br(pre_assert(format.culture_invariant_stability))] | |
| #[brw(magic = -1i8)] |
| // #[binrw] | ||
| // #[br(import(format: &SerializationFormat, size: Option<u32>, struct_type: &FString, class_name: Option<&str>, guid: FGuid, hint_map: &HintMap, path: Path))] | ||
| // #[bw(import(format: &SerializationFormat))] |
There was a problem hiding this comment.
I would prefer to leave FStructProperty derived, because it makes the code substantially easier to read.
I think it should be possible to move the variants that need custom handling into their own struct, and leave the enum itself as derived.
There was a problem hiding this comment.
that seems like it would make matching on the fstructproperty values harder for users, is there a way to inline it with binrw?
There was a problem hiding this comment.
This would not change the shape of FStructProperty.
With binrw, if a variant contains something that can't be derived, you can move just that part into a named type that manually implements the binrw traits, while keeping the parent enum derived.
Instead of:
enum FStructProperty {
Variant(
/* inner value that requires custom BinRead/BinWrite logic */
),
}
// FStructProperty manually implements BinRead/BinWriteyou can do:
#[binrw]
enum FStructProperty {
Variant(InnerType),
}
struct InnerType {
/* fields that require custom BinRead/BinWrite logic */
}
// InnerType manually implements BinRead/BinWriteThe public shape of FStructProperty is still the same kind of enum, so matching on it is unchanged in practice. The difference is just that the custom serialization logic is isolated to the part that actually needs it, rather than manually reimplementing the entire enum's serialization.
binrw, with ~90% of types completely derived.0.11.0Incomplete
FStructProperty::Customwill attempt to parse the property, and in the event that parsing fails,FStructProperty::Unknownwill be used as a generic fallback. This type is distinct fromFProperty::Unknown, because a UE4.5FStructProperty::Unknownhas its own type metadata and struct-guid.