-
Notifications
You must be signed in to change notification settings - Fork 4
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
Final fixes #124
Final fixes #124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some comments
@@ -30,7 +36,10 @@ class RankingEngine: InferenceEngine, InferenceDataDelegate { | |||
} | |||
|
|||
var currentKd: Double { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this can be more clear as to what kd
means but just a nitpick
@@ -52,4 +52,23 @@ final class TotalGamesStatistic: Statistic { | |||
self.init(permanentValue: value, currentValue: current, maxCurrentValue: max) | |||
} | |||
|
|||
func merge<T: Statistic>(with that: T) -> T? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function has multiple repetition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, initially I tried to have a default implementation within Statistics, however, StatisticsDatabase is unable to use the merge function when done that way due to some incompatibility with the (any Statistic.Type) Metatype or something. This is the most robust workaround I found.
TowerForge/TowerForge/Metrics/Statistics/StatisticsEngine.swift
Outdated
Show resolved
Hide resolved
TowerForge/TowerForge/ViewControllers/PlayerStatsViewController.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
Various bug fixes, optimizations, and improvements. Primarily restructuring Storage to support Firebase's async ops. Also update the player stats page.
Completed
Pending