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

Make speed skill consider only the shortest movement distance #15758

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

GoldenMine0502
Copy link
Contributor

idk why this happened.
we were calculating past slider head instead of tail while aim is calculating tail.

it seems to be bug.

trinity homerun HR 824 -> 814

because we changed the roles of movement and jump, this bug happened i think.

umm.. as a result.. idk this is right.

@bdach
Copy link
Collaborator

bdach commented Nov 23, 2021

i am sorry if this is going to sound crass, but from reading the description i do not have the slightest clue what the "bug" here would be or what the "fix" for this is. the opening post has basically a few pieces of what looks like essentially random information that does not help me in understanding this change in any way. i would basically need to stare at it for a few hours just to try and figure out your intentions, and then figure out what the purported fix is fixing exactly.

you've opened several pull requests until now, and none of them have really gotten any serious attention not only from the team at large, but the performance points committee specifically as well. that to me is a clear signal that either your changes do not generally receive agreement from the committee or that the information necessary to explain why the changes were made is not being communicated properly. this may sound harsh but to work with your changes we need to understand your reasoning to address them in any meaningful way. otherwise they will just remain stuck in limbo.

@GoldenMine0502
Copy link
Contributor Author

GoldenMine0502 commented Nov 24, 2021

honestly, i don't know and i can't know why you can't understand it.
i think you guys just dont be concerned/interested. if not, please say anything.
I really want to know how long nerf abused records not predict. so i have few specific results because its from predicting.

the next is my result opinion.

#15323 i understand why this pr is in stucked state. this is too big, so i'm separating it to different pr. and now i feel it needs to be fixed somewhere, but i'm keeping because later pr are related.

#15481 maybe the osu team hasn't checked player's prefer or opinion. especially HR jump, It is too underprevileged to a lot of players. please look at the top rankings. no one did HR except HDDTHR. no one could do 1100pp using only HR(or none). we should balance all mods and types like jump, stream, and alternative. this is from 1.2x bonus on AR11. it is underprevileged on raw HR.

#15485 please discuss or ask. i'm opened to answer it. pp changes need a lot of thinking, philosophy, or etc, so i understand someone can't understand. i will introduce until you all understand, and i will close if my opinion isn't correct.

#15673 do you guys really don't know about how top rankers process sliders? the scorev1 has too many tolerance at sliders. we should remove all of predictable abuses. someone think 5ms, 10ms and they are small and neglectable. but this is very big value on top rankers. you guys checked just +20ms(on current pr) offset nerfs 50pp. this is really the fact small values change a lot.

current pr, same the above. plus, to consistency. aim calculates tail, but speed calculates head. it isn't consistant.

i could know why pp system is all stucked for 2 years.
unknown

@peppy
Copy link
Member

peppy commented Nov 24, 2021

@GoldenMine0502 are you on the pp discord server? it may help to start with discussion there, rather than pull requests out of the blue.

by making pull requests with no statistics, no documentation on how/why the changes were reached, it is very hard for us to act on without investing large sums of our time.

@GoldenMine0502
Copy link
Contributor Author

yes. i'm on both pp and osudev server. anything to say to me, please feel free on any channel.

@apollo-dw
Copy link
Contributor

These appear without any prior discussion on the pp server though, which would be very valuable having first. Note that in this PR alone (which is very small) you don't seem sure about the change you're suggesting - what does that imply for your bigger ones?

umm.. as a result.. idk this is right.

@GoldenMine0502
Copy link
Contributor Author

GoldenMine0502 commented Nov 24, 2021

currVelocity = Math.Max(currVelocity, movementVelocity + travelVelocity); // take the larger total combined velocity.
as you know, this is part of newest aim code. the algorithm adds movementVelocity(or distance) by travelVelocity

but on speed,

double distance = Math.Min(single_spacing_threshold, osuCurrObj.TravelDistance + osuCurrObj.JumpDistance);
this code adds JumpVelocity by travelVelocity.

so i couldnt remove to think that this is an error while changing other codes.
im curious about the purpose. this is why i say idk

btw agreed. i thought theres no relation between osudev and pp

@GoldenMine0502
Copy link
Contributor Author

no way except this picture to introduce.
1

@smoogipoo
Copy link
Contributor

smoogipoo commented Nov 24, 2021

Regarding your diagram, JumpDistance is actually the green line. MovementDistance is actually somewhere closer to the blue line, and is expected to intersect the red line around where the tip of the curve is.

#15774 This PR should help with understanding.

@smoogipoo
Copy link
Contributor

Spreadsheet of these changes: https://docs.google.com/spreadsheets/d/1K2aEWcwpinpcVlNOAAWLuaCv1v4phqOj20hcXpFf0UM/edit#gid=0

I don't think this change is incorrect, fwiw.

@smoogipoo smoogipoo requested a review from a team November 24, 2021 10:36
@smoogipoo smoogipoo changed the title fix adding wrong values on speed Make speed skill consider only the shortest movement distance Nov 24, 2021
@bdach
Copy link
Collaborator

bdach commented Nov 24, 2021

Going to hold off on re-checking this one until #15774 is merged and this pull is updated to use the new names.

@GoldenMine0502
Copy link
Contributor Author

yes the curve is for understanding. it is originally TravelDistance + MovementDistance (straight line).

@GoldenMine0502
Copy link
Contributor Author

Regarding your diagram, JumpDistance is actually the green line. MovementDistance is actually somewhere closer to the blue line, and is expected to intersect the red line around where the tip of the curve is.

#15774 This PR should help with understanding.

btw JumpDistance = (BaseObject.StackedPosition * scalingFactor - lastCursorPosition * scalingFactor).Length;
means blue line in current version.

@smoogipoo
Copy link
Contributor

btw JumpDistance = (BaseObject.StackedPosition * scalingFactor - lastCursorPosition * scalingFactor).Length;
means blue line in current version.

No because of the lastCursorPosition part of it. For a slider, that's going to be close to the tail.

@GoldenMine0502
Copy link
Contributor Author

ah okay my fault

@Wieku
Copy link
Contributor

Wieku commented Nov 25, 2021

no way except this picture to introduce. 1

I had to correct this. This is how it works in huge oversimplification:
image

@smoogipoo smoogipoo merged commit 74db8da into ppy:master Dec 7, 2021
smoogipoo added a commit to smoogipoo/osu that referenced this pull request Jan 17, 2022
This reverts commit 74db8da, reversing
changes made to 86eacfd.
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.

7 participants