search

siku2 / script.service.sponsorblock

Kodi add-on for SponsorBlock
MIT License
124 stars 15 forks source link

Sort received segments by start time #18

Closed macearl closed 3 years ago

macearl commented 3 years ago

This sorts the received sponsor segments by start time to prevent the addon only skipping the first segment in the list which is quite often (or possibly always?) the outro.

Not sure why the assertion does not stop further steps.

This is a pretty simple fix, there might be better ways for this, but as I'm debugging and creating this PR through my phone I think it's good enough for now :)

siku2 commented 3 years ago

Hah, I always just assumed that the SponsorBlock API returned segments in order of appearance and then I never actually bothered to check. I'm fairly confident that Kodi runs the Python script with the optimize flag which eliminates assert statements which explains why that never pulled the breaks.

Either way, this is fantastic! One small request though, could you move the sorting to the get_skip_segments function? I feel like that function should return the segments in order.

Oh and is there something else you're trying to debug right now or should I release a patch right after this is merged?

macearl commented 3 years ago

Sure, it probably does fit better in the API file.

Most of my initial issues are fixed now, i just thought that different settings produced different behavior but that was just due to different sponsor segment ordering for different videos.

There do seem to be some more problems related to skipping multiple sections back to back (sponsor segment directly followed by either an intro or outro). I'll try and have another look at those and will probably open another issue and/or PR for it if i find out more about it.

siku2 commented 3 years ago

Ah yes, that's very possible. Before categories were a thing overlaps werent supposed to happen (IIRC) but now that's obviously no longer true.

The sanity check also checks for this but we should obviously handle this properly.

siku2 commented 3 years ago

Alright, I added a little something in e33cf04 that should hopefully make it so that back to back segments with overlap are handled properly. Let me know if this is what you were referring to. I released this together with the changes made in your PR as a new patch version.

macearl commented 3 years ago

seems i was a bit too quick with my additional PR, at a first glance that should fix it. I merged the segments into each other instead. I'll try your version tomorrow.

Probably only theoretical, or at least really rare but for segments which are a couple of ms apart this would still do multiple skips right? I just arbitrarily picked half a second as the cut off point, if the segments are less than that apart i merged them together to prevent multiple skips which would feel rather choppy

siku2 commented 3 years ago

Good point, I didn't really think about that. Sounds like a great addition. IMO, the cut off point should be configurable rather than having a specific hard coded value.