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

Timer splits for stages and ordered/required checkpoints #160

Merged
merged 4 commits into from
Nov 30, 2024

Conversation

Panzerhandschuh
Copy link
Member

@Panzerhandschuh Panzerhandschuh commented Nov 26, 2024

I will come back to this and add support for unordered/optional checkpoints and run stats comparisons.

Checks

  • I have thoroughly tested all of the code I have modified/added/removed to ensure something else did not break
  • I have followed semantic commit messages e.g. feat: Add foo, chore: Update bar, etc...
  • My branch has a clear history of changes that can be easy to follow when being reviewed commit-by-commit
  • My branch is functionally complete; the only changes to be done will be those potentially requested in code review
  • All changes requested in review have been fixuped into my original commits.
  • Fully tokenized all my strings (no hardcoded English strings!!) and supplied bulk JSON strings below

@Panzerhandschuh Panzerhandschuh force-pushed the feat/new-timer-splits branch 4 times, most recently from 77f9f17 to 21118e3 Compare November 26, 2024 21:06
@PeenScreeker
Copy link
Member

lgtm

scripts/common/timer.ts Outdated Show resolved Hide resolved
Comment on lines 87 to 89
for (const panel of splitPanels.filter((_, i) => splitPanels.length - i > this.maxActiveSplits)) {
panel.RemoveAndDeleteChildren();
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think

splitPanels
  .filter((_, i) => splitPanels.length - i > this.maxActiveSplits))
  .forEach(panel => panel.RemoveAndDeleteChildren());

is much more readable. Though it looks like you could be doing a C-style for loop here instead.

scripts/hud/comparisons.ts Outdated Show resolved Hide resolved
scripts/pages/end-of-run/end-of-run.ts Show resolved Hide resolved
Comment on lines 331 to 333
// statsComparisons: baseRunSplits.trackStats.map(
// (stat, j) => new RunStatsComparison(stat, compareZone.stats[j])
// )
Copy link
Member

Choose a reason for hiding this comment

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

By commenting this out, you've effectively made this statsComparison field totally non-existent, despite there being loads of other code in this PR that uses it. It makes sense that you had to comment it out, we've completely changed how stats work, but that means you need to go through and change all the stats usage to use the new RunStats data structure passed in from C++.

scripts/common/timer.ts Show resolved Hide resolved
scripts/pages/end-of-run/end-of-run.ts Show resolved Hide resolved
@tsa96
Copy link
Member

tsa96 commented Nov 28, 2024

This might not fit into current code perfect but for RunStatsComparison you could do something like this:

const RunStatsUnits: Record<keyof RunStats, string> = {
	maxOverallSpeed: 'ups',
	maxHorizontalSpeed: 'ups',

	overallDistanceTravelled: 'units',
	horizontalDistanceTravelled: 'units',

	jumps: 'jumps',
	strafes: 'strafes'
};

export interface RunStatsComparison {
	name: string;
	unit: string;
	baseValue: number;
	comparisonValue: number;
	diff: number;
}

export function GenerateStatsComparison(baseStats: RunStats, comparisonStat: RunStats): RunStatsComparison[] {
	return (Object.keys(baseStats) as Array<keyof RunStats>).map((key) => ({
		name: key,
		unit: RunStatsUnits[key],
		baseValue: baseStats[key],
		comparisonValue: comparisonStat[key],
		diff: baseStats[key] - comparisonStat[key]
	}));
}

@Panzerhandschuh Panzerhandschuh force-pushed the feat/new-timer-splits branch 2 times, most recently from da8e5f3 to 82173f7 Compare November 29, 2024 18:16
Copy link
Member

@tsa96 tsa96 left a comment

Choose a reason for hiding this comment

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

Was trying to understand what was going on in the constructor of the Comparison class and realised it's never even getting used.

We shouldn't need to keep Run_OLD around given the new RunSplits stuff, and so I don't really see the point of this class. The only thing that's still obviously relevant are generateSplits (though never gets used atm) and generateSubsegmentSplit. These are static methods and don't really have anything to do with comparisons, so they should just be plain functions.

We could pull this all out of classes and use constructors, but that's absolutely not a requirement in JS/TS, and since we have several instances of constructor-like functions that return arrays of what they should be constructing, it seems easier to skip using classes and just use interfaces + pure functions. We can then name the import of common/timer.ts in some places to scope the functions better.

Sorry to piggy-back your work but I really want to get the data structures organised well here and avoid using any of these unnecessary old classes. I've just spent a half hour reorganising stuff a little doing the WIP refactoring of the endofrun code. Let's merge what you have here, then I'm gonna push my WIP stuff on top of that, and I'd appreciate it if you or anyone else worked off of there when doing end of run

@tsa96 tsa96 merged commit 03f030f into feat/mom-0.10 Nov 30, 2024
1 check passed
@tsa96 tsa96 deleted the feat/new-timer-splits branch November 30, 2024 17:23
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.

3 participants