LiveSplit / livesplit-core

livesplit-core is a library that provides a lot of functionality for creating a speedrun timer.
https://livesplit.org/
Apache License 2.0
209 stars 58 forks source link

Refactor Segment History #251

Open CryZe opened 4 years ago

CryZe commented 4 years ago

We use the sign of the segment history attempt IDs to differentiate whether they belong to a real attempt or a generated, algorithmic one. This causes lots of weird issues and is not idiomatic Rust at all. We should refactor this at some point.

wooferzfg commented 4 years ago

This is also related to https://github.com/LiveSplit/LiveSplit/issues/995

wooferzfg commented 4 years ago

I wrote up a doc that's a mix of a new design and an explanation of the stuff we already do: https://docs.google.com/document/d/1DzA_a0Qz_kHkX6_PUDwDSMZG5GMNlDVFuMC5sO93hac/edit?usp=sharing

CryZe commented 4 years ago

Nice, I'd probably want to entirely get rid of the negative IDs while we are at it. The generated="true" and co. should be enough to differentiate them.

wooferzfg commented 4 years ago

The generated segment history elements still need some sort of IDs though, and they should probably be different from the IDs of the real attempts.

CryZe commented 4 years ago

Yeah, but duplicating the information in the XML isn't a great idea. We'd need to verify that the XML doesn't contain any information that breaks the invariants (negative but not generated, positive but generated), if we could just not have any way to do so in the first place. In the actual application we could either transform it back to negative numbers or something else then (different segment histories, or an enum, ...).

vortexofdoom commented 7 months ago

I opened #763 a couple days ago and then ended up seeing this, and it seems like a good thing to add to this discussion.

I have some questions about the generated elements. Is there any need for those generated indices to be sorted? The main value of having sorted indices seems to be for finding different segments across a single run. If not, we could have SegmentHistory have a BTreeMap<u32, Time> and a Vec<Time> for the generated ones. If they should both be sorted we could either make a tagged enum where both variants hold a u32, or make a newtype wrapper around a u32 that still used the sign bit as a flag, and used the other 31 bits to store the ID (Basically just a custom ones complement 32 bit type). This could be done while remaining const, and you could either store them as two separate sets of attempts or put them together, and implement Ord for the representation which could sort them however we wanted. We could keep the XML representation as negatives trivially as well, since you just cast it back and it's signed. Or you could just mask that off and have it be a number like in the google doc, adding a generated=True flag based on only the sign bit.

Mocked up a basic implementation outline:

enum SplitEnd {
    Finished(Time),
    Skipped,
    Reset,
}

#[derive(PartialEq, PartialOrd, Ord)]
struct SegmentAttemptId(u32);

impl SegmentAttemptId {
    // Uses sign bit to mark as generated while leaving 31 bit space for id numbers
    const GENERATED_FLAG: u32 = 1 << 31;

    const fn generated(&self) -> bool {
        self.0 & Self::GENERATED_FLAG != 0
    }
}

struct SegmentHistory(BTreeMap<SegmentAttemptId, SplitEnd>);

impl SegmentHistory {
// ....
    fn real_runs(&self) -> Range<'_, SegmentAttemptId, SplitEnd> {
        // starts at attempt 1 and finds all attempts with id < Self::GENERATED_FLAG
        self.0.range((1..Self::GENERATED_FLAG).map(SegmentAttemptId))
    }
// ....    
}