Skip to content
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

Demo rewrite #74

Merged
merged 12 commits into from
Jan 6, 2024
Merged

Demo rewrite #74

merged 12 commits into from
Jan 6, 2024

Conversation

Patiga
Copy link
Collaborator

@Patiga Patiga commented Mar 17, 2023

Some relatively minor things like naming might still need to be changed
I haven't tested the demo writing yet, would be appreciated if someone else could do that.
I'm ofc open to changes :)

@Patiga
Copy link
Collaborator Author

Patiga commented Mar 19, 2023

I think the snapshot would be easier to work with, if it wasn't exposed as an Iterator or single HashMap, but instead as a struct with a HashMap for each snap object type.

struct Snapshot {
    player_input: HashMap<u16, PlayerInput>,
    projectile: HashMap<u16, Projectile>,
    laser: HashMap<u16, Laser>,
    ...
}

Each field would map each id to the object.
That struct could be generated using the gamenet python scripts, which I'm not familiar enough with.
(The fields could be suffixed with an s for a more intuitive plural form, but that wouldn't work all that well with all names)

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 8, 2023

Rebased and with a tool to resave demos, in order to detect differences in parsing/saving.
Currently, it doesn't produce the same file, but visually the demo seemed correct. It would require further investigation, maybe Kaitai could help.

I think it is ready to merge for now. The gamenet and demo crate will be more actively used by me now, so maybe I'll need to develop this further soon.

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 8, 2023

Fixed failed build

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 10, 2023

I'm not sure what to do about the 2 failing checks. They appear to be about the Rust edition 2021(?).
Should I try to lower the Rust version @heinrich5991?

@heinrich5991
Copy link
Owner

heinrich5991 commented Dec 13, 2023

Why does it fail with nightly? 🤔

You can ignore the failure with old Rust for ow.

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 13, 2023

A simple cargo update fixed the failed build. If preferred, I could probably also only run the command for proc_macro.

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 29, 2023

After the latest change, which has nothing to do with any dependencies, rustc-serialize fails to build on ubuntu-latest, nightly???
I'll try another cargo update -p rustc-serialize, but I feel like I'm fixing symptoms of something I have no control over.

@Patiga
Copy link
Collaborator Author

Patiga commented Dec 29, 2023

Welp, that fixed it again

@Patiga Patiga force-pushed the demo-rewrite branch 3 times, most recently from 3d7d301 to 59359eb Compare December 30, 2023 11:42
Copy link
Owner

@heinrich5991 heinrich5991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Is this ready to merge?

I left a couple of nits, but this can be merged without fixing them. In this case, I'd go over these myself and make a PR. :)

keyframe: bool,
},
Snapshot(&'a ArrayVec<[u8; MAX_SNAPSHOT_SIZE]>),
SnapshotDelta(&'a ArrayVec<[u8; MAX_SNAPSHOT_SIZE]>),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rather &'a [u8], I see no reason to make the type more specific.

}

let mut builder = snap.recycle();
for size in &self.sizes {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for size in &self.sizes {
for &size in &self.sizes {

gets rid of the need to dereference below.

Snapshots provide the full game state. In demos, they are used to provide
additional entry points for the stream of snapshot deltas.

- `data_size` is a positive signed 32-bit integer describing the amount of
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below: nonnegative or positive?
(0 is nonnegative but not positive.)

Comment on lines +69 to +70
The `chunk_data` of snapshots and snapshot deltas goes through variable-int
compression once, and twice for messages.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "twice" mean decoded as ints twice, and the resulting bytes are then unpacked using TW variable int encoding a third time if the message has an int?

Or does it only go through int decoding once if the message doesn't contain any integers?

Inner(#[from] reader::ReadError),
#[error("{0:?}")]
Snap(snapshot::snap::Error),
#[error("Uuid item has an incorrect size")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[error("Uuid item has an incorrect size")]
#[error("UUID item has an incorrect size")]

Snap(snapshot::snap::Error),
#[error("Uuid item has an incorrect size")]
UuidItemLength,
#[error("Uuid type id that is not registered")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[error("Uuid type id that is not registered")]
#[error("UUID type ID that is not registered")]

@heinrich5991
Copy link
Owner

I didn't know about the binrw crate. It looks pretty nice: https://docs.rs/binrw/0.13.3/binrw/.

Patiga added 9 commits January 6, 2024 01:06
Also add `thiserror` dependency
doc/demo Add variable-int compression
Abstracts away compression, snapshot parsing and applying of deltas
Usage: Find out if the Reader and Writer work as expected.
Currently, it does produce a different file on a randomly selected demo.
However, I didn't see any visual differences.
Patiga and others added 3 commits January 6, 2024 01:06
`cargo update -p proc-macro2@1.0.51`
(two different versions exist in the dependency tree)
Grants access to all header data.
Fixes pub attributes and exports.
Adjust names of methods on `Reader`.
@heinrich5991 heinrich5991 merged commit 1b2be88 into heinrich5991:master Jan 6, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants