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

Code refactoring Aim and Speed #15485

Closed
wants to merge 30 commits into from

Conversation

GoldenMine0502
Copy link
Contributor

@GoldenMine0502 GoldenMine0502 commented Nov 5, 2021

this pr is for refactoring.

we could see simple aim and speed now.

it seems to have slight difference on diff calc.

i could check two factors:

  1. recent slider nerf
  2. AimVelocity now starts to calculate second note, originally third.
    .. etc? (if you find additional problem it is my problem)

overall i think its not fatal problem.

recent off by:
Test(1.448475413914554d,"zero-length-sliders")
Expected: 1.4484754139145539d +/- 1.0000000000000001E-05d
But was: 1.4804738333333043d
Off by: -0.031998419418750368d

TestClockRateAdjusted(1.7548875851757628d,"zero-length-sliders")
Expected: 1.7548875851757628d +/- 1.0000000000000001E-05d
But was: 1.806793929165774d
Off by: -0.05190634399001115d

TestClockRateAdjusted(8.938255920868981d,"diffcalc-test")
Expected: 8.9382559208689809d +/- 1.0000000000000001E-05d
But was: 8.9383248067074295d
Off by: -6.888583844855134E-05d

@GoldenMine0502
Copy link
Contributor Author

GoldenMine0502 commented Dec 11, 2021

we could see just simple code on Aim and Speed.

it will act for code related to the first conversation. after merged, i will do for it.

@GoldenMine0502
Copy link
Contributor Author

GoldenMine0502 commented Dec 12, 2021

it seems to have slight difference on diff calc.

i could check two factors:

  1. recent slider nerf
  2. AimVelocity now starts to calculate second note, originally third.
    .. etc? (if you find additional problem it is my problem)

overall i think its not fatal problem.

recent off by:
Test(1.448475413914554d,"zero-length-sliders")
Expected: 1.4484754139145539d +/- 1.0000000000000001E-05d
But was: 1.4804738333333043d
Off by: -0.031998419418750368d

TestClockRateAdjusted(1.7548875851757628d,"zero-length-sliders")
Expected: 1.7548875851757628d +/- 1.0000000000000001E-05d
But was: 1.806793929165774d
Off by: -0.05190634399001115d

TestClockRateAdjusted(8.938255920868981d,"diffcalc-test")
Expected: 8.9382559208689809d +/- 1.0000000000000001E-05d
But was: 8.9383248067074295d
Off by: -6.888583844855134E-05d

@peppy
Copy link
Member

peppy commented Dec 13, 2021

Would appreciate if someone with more knowledge of this change can provide a better PR title. The current one is... hard to parse.

@bdach
Copy link
Collaborator

bdach commented Dec 13, 2021

If I did have such knowledge I would have already changed it. The author of this pull is pretty much working solo, and due to a seemingly insurmountable communication barrier I'm having an extremely difficult time comprehending pretty much all of this thread.

@GoldenMine0502 GoldenMine0502 changed the title structure of preskills basecode Code refactoring Aim and Speed Dec 13, 2021
@smoogipoo
Copy link
Contributor

  • Why is there a "PreStrainSkill" class again? I thought I mentioned that I don't want a new class hierarchy here.
  • ProcessInternal shouldn't be public. It's "internal" for a reason.
  • This isn't really much of a refactoring, you've just added one more indirection but all the classes are still completely tied together. This doesn't for example, allow you to use the AimVelocity pre-strain-skill inside both the Speed and Aim skills.

I really don't see what we gain out of this... Am I alone in thinking this way?

@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 13, 2021

Sorry, but I'm going to take executive action here and close this PR as not the direction we want to go.

Feel free to start a discussion and say your thoughts on where things should go / what you want to be able to do with the diffcalc system before taking on such refactorings. That way we can all agree on the path forward prior to a PR being made.

The system is already far too complex to facilitate even more complexity.

@smoogipoo smoogipoo closed this Dec 13, 2021
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.

4 participants