Demo Seeking, Attempt 3 - #148
Conversation
heinrich5991
left a comment
heinrich5991
left a comment
There was a problem hiding this comment.
Looks very well-thought out. Thanks for the PR.
It'd be best if this wasn't a binary, but a test that was automatically run on changes, but I don't have a repository of demos right now and wouldn't know where to store them without bloating this git repository, so whatever. The binary is a good building block for such a test.
| if matches!(marker, TickMarker::Delta(_)) && keyframe { | ||
| warn.warn(Warning::DeltaTickKeyframe); | ||
| keyframe = false; | ||
| } |
There was a problem hiding this comment.
What was the motivation for adding this fail-safe? Have you observed this happening in real demos?
There was a problem hiding this comment.
My motivation is that keyframe == true implies/suggests that we can jump to this chunk in a demo later on and just start reading from there. A delta tick does not fulfil this promise, so simply setting keyframe = false in that case seemed like a sane action.
Until now, I have not checked any demos for this behavior. I now checked a few (~5) demos and this doesn't seem to be a thing. Would you rather have this be an error or be handled differently?
|
For now I only exposed |
|
ah, gotta rebase on master |
849ffcf to
4a593ba
Compare
|
Fixed three inconsistencies:
|
|
imo this can be merged now, would also appreciate a crates.io bump. |
2 participants
Here I just went with the existing readers and added the required functionality.
I like the relatively small changes, and think this is the best solution for now, and also mergable.
In the interpreted reader interface I needed to expose if a tick chunk is marked as a keyframe. This is a breaking change sadly.
The only immediate change I would recommend alongside this one is to rename the
ddnetmodule to something else, as it is generic over the protocol.