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

Livesplit - Provide a TimeCalc URL for Season/Trilogy speedruns #42

Merged
merged 5 commits into from Dec 12, 2022

Conversation

alex73630
Copy link
Contributor

Copy link
Contributor

@fu5ha fu5ha left a comment

Choose a reason for hiding this comment

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

Very cool idea, I support it! Unfortunately, there's an important corner case that isn't handled yet:

If you "complete" a mission, but have red guns, or otherwise didn't complete your objective, the autosplitter allows you to just restart the level and it will unsplit. The timecalc info tracking should support this case as well. See lines 111-130 for where this is handled for livesplit itself (i.e. where the unsplit call happens). You should be able to just get the latest entry in the timeCalcEntries and set its isComplete to false, if I'm not missing anything.

Copy link
Member

@LennardF1989 LennardF1989 left a comment

Choose a reason for hiding this comment

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

My 2 cents.

components/types/types.ts Outdated Show resolved Hide resolved
components/discordRp.ts Show resolved Hide resolved
- Return 2 URLs if there is not enough lines to fit all the resets
- Print time as mm:ss when time is over 60s
- Include resetMinimum and time under 1s logics in TimeCalc output
- Handle unsplit logic in TimeCalc
Copy link
Contributor

@fu5ha fu5ha left a comment

Choose a reason for hiding this comment

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

I didn't really look too hard at the link generation code, but other than that looks good other than comments :)

components/livesplit/liveSplitManager.ts Outdated Show resolved Hide resolved
components/livesplit/liveSplitManager.ts Outdated Show resolved Hide resolved
Copy link
Member

@RDIL RDIL left a comment

Choose a reason for hiding this comment

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

Link gen + discord RP changes LGTM, but Fusha's feedback still remains.

- Updated _addMissionTime to always return a floored time
- Updated _formatSecondsToTime to better match other code patterns in module
- Removed debug logs entries
@alex73630 alex73630 requested a review from fu5ha December 10, 2022 10:04
@alex73630
Copy link
Contributor Author

I updated the code following @fu5ha recommendations and now waiting on approval.

Thanks everyone for the feedback!

@alex73630 alex73630 changed the title Livesplit TimeCalc URL Livesplit - Provide a TimeCalc URL for Season/Trilogy speedruns Dec 10, 2022
@fu5ha fu5ha removed the request for review from LennardF1989 December 12, 2022 15:17
@fu5ha fu5ha merged commit 240436f into v5 Dec 12, 2022
@RDIL RDIL deleted the livesplit-timecalc branch December 12, 2022 15:36
scrungofan pushed a commit to scrungofan/Peacock that referenced this pull request Dec 25, 2022
…eacockproject#42)

* wip(livesplit): store individual times for timecalc sheet

* feat(livesplit): generate timecalc link at run end

* chore: move livesplit typings in its own types file

* feat(livesplit): complete timecalc feature
- Return 2 URLs if there is not enough lines to fit all the resets
- Print time as mm:ss when time is over 60s
- Include resetMinimum and time under 1s logics in TimeCalc output
- Handle unsplit logic in TimeCalc

* feat(livesplit): couple of changes over pr comments
- Updated _addMissionTime to always return a floored time
- Updated _formatSecondsToTime to better match other code patterns in module
- Removed debug logs entries
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.

None yet

4 participants