-
Notifications
You must be signed in to change notification settings - Fork 7
multi-game support and flesh out cli describe command for more formats #49
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?
Conversation
|
For multi-game support this looks like an okay first-pass, but I expect the design to change quite a bit. For types that exist across several games we'll probably want some sort of proc macro that generates variants of that type: #[versioned(by = Game)]
#[repr(C, packed)]
pub struct SomeType {
#[added(EldenRing)]
#[removed(Nightreign)]
some_value: U32,
}which would generate We can see how it impacts downstream API consumers already: if let PartData::EldenRing(parts::elden_ring::PartData::DummyAsset(_)) = part.part {
return None;
}For this example I think the direction towards the ideal API is something like: // Generated types from something like the macro above:
pub trait DummyAssetPart {}
pub struct EldenRingDummyAssetPart {}
pub struct NightreignDumymAssetPart {}
let Ok(data) = part.data::<DummyAssetPart>(); else { panic!("not a dummy asset part!"); };
let some_value : Option<u32> = data.some_value();
let some_value_raw = data.downcast::<EldenRingDummyAssetPart>(); |
| /// | ||
| /// Example: Given a collection of events and the event type SignPool, | ||
| /// return a vector containing only SignPool events. | ||
| fn of_type( |
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.
Within the formats crate we try to avoid allocating unless absolutely necessary. Can we make this return an iterator instead of a collection?
| #[arg(short, long, required = false, value_delimiter = ',', help = "Chain of nested bnd names. Required to describe a file therein.\nExamples:\n Describe a tae inside the anibnd of an objbnd:\n -n obj\\o000100.objbnd.dcx, o000100.anibnd tae o000100.tae\n Describe a flver inside a chrbnd:\n -n chr\\c3000.chrbnd.dcx flver c3000.flver")] | ||
| nested_bnd_names: Vec<String>, | ||
|
|
||
| #[arg(value_enum)] |
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.
You can use regular Rust comments to write the help message for options, i.e.
/// Chain of nested bnd names. Required to describe a file therein.
///
/// Examples:
/// Describe a tae inside the anibnd of an objbnd: -n obj\o000100.objbnd.dcx
/// ...
#[arg(short, long, required = false, value_delimiter = ',')]
nested_bnd_names: Vec<String>,|
FWIW, to fix the bulk of these CI complaints automatically: |
Thanks for the feedback. It's an approach I hadn't considered as a new rust programmer and I definitely agree that my current approach isn't ideal. I'll have to look into how macros work in more detail. |
|
@GompDS unless you've already started working on something for this (we don't need to tackle that as part of this PR btw, it's going to be a pretty involved piece of work with a lot of design decisions) I'm going to pull this branch and see what I can do to bring in some form of abstraction. |
|
I agree it would be good if we could separate the updates to describe ive done and the multi game support stuff. It seems like two different things to tackle. |
|
Alright, here's an initial sketch of how this is working: #[versioned(versions = [eldenring, nightreign])]
pub enum PointData<'a> {
Other,
GroupDefeatReward {
unk0: I32<LE>,
unk4: I32<LE>,
unk8: I32<LE>,
unkc: I32<LE>,
unk10: I32<LE>,
unk14: [I32<LE>; 8],
unk34: I32<LE>,
unk38: I32<LE>,
unk3c: I32<LE>,
unk40: I32<LE>,
unk44: I32<LE>,
unk48: I32<LE>,
unk4c: I32<LE>,
unk50: I32<LE>,
unk54: I32<LE>,
unk58: I32<LE>,
unk5c: I32<LE>,
},
#[added(nightreign)]
UserEdgeEliminationExterior {
// we want the structs inline, we'll generate separate structs based on the version permutations
},
// ...
}This generates quite a few things. We get shared structs where all versions share a data structure: pub struct GroupDefeatReward {
unk0: I32<LE>,
unk4: I32<LE>,
unk8: I32<LE>,
unkc: I32<LE>,
unk10: I32<LE>,
unk14: [I32<LE>; 8],
unk34: I32<LE>,
unk38: I32<LE>,
unk3c: I32<LE>,
unk40: I32<LE>,
unk44: I32<LE>,
unk48: I32<LE>,
unk4c: I32<LE>,
unk50: I32<LE>,
unk54: I32<LE>,
unk58: I32<LE>,
unk5c: I32<LE>,
}We generate a struct for the contents of enum variants (see why below): mod nightreign {
pub struct UserEdgeEliminationExterior {
}
}We generate traits to expose a common API across multiple versions: pub trait UserEdgeEliminationExterior {
fn some_field(&self) -> ...;
}
impl UserEdgeEliminationExterior for nightreign::UserEdgeEliminationExterior {}I'll split out the describe/versioning changes from this PR when I get to opening one on top of this. |
|
If I'm understanding this correctly, would it mean that there is also an eldenring::GroupDefeatReward and a nightreign::GroupDefeatReward? or are those variants only implemented when an added or removed macro is used? |
This is correct, the generated enum would look something like: pub enum NightreignPointParam {
GroupDefeatReward(common::GroupDefeatReward /* actual struct with the fields */),
UserEdgeEliminationExterior(nightreign::UserEdgeEliminationExterior)
}And we'd also have a GroupDefeatReward {
unk0: I32<LE>,
unk4: I32<LE>,
unk8: I32<LE>,
unkc: I32<LE>,
unk10: I32<LE>,
unk14: [I32<LE>; 8],
unk34: I32<LE>,
unk38: I32<LE>,
unk3c: I32<LE>,
unk40: I32<LE>,
unk44: I32<LE>,
unk48: I32<LE>,
unk4c: I32<LE>,
unk50: I32<LE>,
unk54: I32<LE>,
unk58: I32<LE>,
unk5c: I32<LE>,
#[added(version = "bloodborne2")]
oh_no: I32<LE>,
},This forces us to split the once common struct into at least 2 versions and is a breaking change if users are consuming this directly. Instead, we'd have a trait we can program against and that new field results in an accessor that returns an optional value (note we'll probably want to make it something other than Option to discriminate from truly optional values) pub trait GroupDefeatReward {
// ...
fn oh_no(&self) -> Option<i32>; |
|
Ok cool. My main concern here was that common structs could vary between games, so I'm glad to see you've considered that. Seems like a good approach so far. |
|
Great, that sounds good. I think it's best we separate that and the multi game support now anyways. |
This pull-request does a couple things:
In regards to multi-game support:
In regards to describing formats inside nested bnds:
One more thing:
Since this is my first pr to the repo, and I'm new to rust programming, please let me know if there's anything I should adjust or what not. Thanks.