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

Refactor skills - move logic out of inheritance hierarchy #15010

Closed
wants to merge 10 commits into from

Conversation

joseph-ireland
Copy link
Contributor

@joseph-ireland joseph-ireland commented Oct 9, 2021

Split out the implementations of strain peaks, decaying values, and exponential weighted sums into their own functions apart from the Skill inheritance hierarchy.

In preparation for a PR to add a list of attributes for each hit object - see #14934

The simplest implementation of that would requre skills to have their own Process(DIfficultyHitObject) function in order to have access to current and cumulative strain. Leaving the inheritance hierarchy as is would result in a confusing ping pong of method calls up and down the inheritance hierarchy.

Note: This affects difficulty values. Some of the simplifications make it slightly awkward to keep the initial strain value as 1, so I change the starting value to zero - see the relevant commit below.

It doesn't make sense for strain to start at 1 anyway - it's basically an unconfigurable strain for the first hit object which isn't affected by SkillMultiplier

bdach
bdach previously requested changes Oct 9, 2021
osu.Game/Rulesets/Difficulty/Utils/DecayingStrainPeaks.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Utils/SectionPeaks.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Utils/DecayingValue.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Utils/DecayingValue.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Skills/StrainSkill.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Skills/StrainSkill.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Difficulty/Skills/StrainSkill.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Taiko/Difficulty/Skills/Rhythm.cs Outdated Show resolved Hide resolved
@bdach bdach requested a review from a team October 9, 2021 11:57
@bdach
Copy link
Collaborator

bdach commented Oct 9, 2021

requesting review from committee wrt the initial strain change

@peppy
Copy link
Member

peppy commented Oct 9, 2021

Why is this adjusting values and also majorly changing structure? Why are those two things not done separately?

@bdach
Copy link
Collaborator

bdach commented Oct 9, 2021

@peppy have already brought this up in #15010 (comment). i do agree it may be better to split off the initial strain change to a separate PR.

@bdach
Copy link
Collaborator

bdach commented Oct 10, 2021

Several merge conflicts to be resolved here, unfortunately :(

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

The structure seems fine to me. A few nitpicks with the docs but I consider the blockers resolved.

osu.Game/Rulesets/Difficulty/Skills/StrainSkill.cs Outdated Show resolved Hide resolved
@@ -9,7 +9,7 @@
namespace osu.Game.Rulesets.Difficulty.Skills
{
/// <summary>
/// A bare minimal abstract skill for fully custom skill implementations.
/// Process the <see cref="DifficultyHitObject"/> in a map and produce a difficulty value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unsure if this is a better description than the previous one. They're both pretty barebones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that the previous docs pretty much say public abstract class Skill in words. I can revert or even just delete the doc

@bdach bdach dismissed their stale review October 11, 2021 19:17

blockers resolved

@GoldenMine0502
Copy link
Contributor

GoldenMine0502 commented Nov 5, 2021

I can't understand why straindecaybase and sectionlength should go into parameter.
why you did it? and what do we anticipate through it?

@smoogipoo
Copy link
Contributor

Gonna close this for the time being since there's going to be some heavy merge conflicts. Feel free to rebase and open a new PR as I still think it's a good direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants