Tillerino / Tillerinobot

352 stars 112 forks source link

When using "!with DT", modify the BPM to BPMx1.5 #5

Closed ThePooN closed 10 years ago

ThePooN commented 10 years ago

i will maybe do a PR for this if i have some time, i should base it on which branch?

EDIT: same with the map duration & stars.

Tillerino commented 10 years ago

That would be great thx. Just base it on the master branch.

If you do this, please adjust the total time and AR as well, and do it for all mods (that would be HT, DT, EZ and HR). You can use org.tillerino.osuApiModel.OsuApiBeatmap.calcAR(double, long), to adjust AR. Please remove star difficulty from the display. We don't know how to adjust it correctly, and instead of putting in time to do find out how, or do some approximate adjustments, we can just take it out entirely. I think players who use the bot aren't interested in star difficulty anyway.

On Sun, Sep 28, 2014 at 10:23 PM, ThePooN notifications@github.com wrote:

i will maybe do a PR for this if i have some time, i should base it on which branch?

— Reply to this email directly or view it on GitHub https://github.com/Tillerino/Tillerinobot/issues/5.

Tillerino commented 10 years ago

Actually, please leave star difficulty for nomod. No need to remove that.

On Mon, Sep 29, 2014 at 8:55 AM, Tillmann Gaida tillmann.gaida@gmail.com wrote:

That would be great thx. Just base it on the master branch.

If you do this, please adjust the total time and AR as well, and do it for all mods (that would be HT, DT, EZ and HR). You can use org.tillerino.osuApiModel.OsuApiBeatmap.calcAR(double, long), to adjust AR. Please remove star difficulty from the display. We don't know how to adjust it correctly, and instead of putting in time to do find out how, or do some approximate adjustments, we can just take it out entirely. I think players who use the bot aren't interested in star difficulty anyway.

On Sun, Sep 28, 2014 at 10:23 PM, ThePooN notifications@github.com wrote:

i will maybe do a PR for this if i have some time, i should base it on which branch?

— Reply to this email directly or view it on GitHub https://github.com/Tillerino/Tillerinobot/issues/5.

ThePooN commented 10 years ago

i've done it, excepted for the AR. https://github.com/ThePooN/Tillerinobot/commit/a791e0503cd54d6b6341eba9d40c5ccdbc9ad790

i don't know how to use org.tillerino.osuApiModel.OsuApiBeatmap.calcAR(double, long), you specified the parameters type, but what should i put in parameter? i don't get it.

EDIT : woops, wrong value. https://github.com/ThePooN/Tillerinobot/commit/edb57979bb3a995a36839ecec3e1fdbdbd7d530f

Tillerino commented 10 years ago

I don't understand the calcAR confusion. What parameter type?

Could you use (2d / 3) and (4d / 3) instead of 0.66 and 1.33 respectively, please?

ThePooN commented 10 years ago

done.

what parameters should I put in calcAR()? i need to put a double and a long ; but what they are? my english is limited so it's hard to explain :-P

Tillerino commented 10 years ago

ok, no problem :)

actually, I see that you can just call beatmap.getApproachRate(long) with the currently selected mods. You can get the mods from PercentageEstimates.getMods()

On Tue, Sep 30, 2014 at 10:19 PM, ThePooN notifications@github.com wrote:

done.

what parameters should I put in calcAR()? i need to put a double and a long ; but what they are? my english is limited so it's hard to explain :-P

— Reply to this email directly or view it on GitHub https://github.com/Tillerino/Tillerinobot/issues/5#issuecomment-57376061 .

ThePooN commented 10 years ago

commited, it should work. :)

Tillerino commented 10 years ago

I don't think it will. It has occurred to me that you're not even compiling the project, is that possible? ;)

ThePooN commented 10 years ago

what do you mean by this? i can't compile, i don't have your library...

the only errors i have are "XXX cannot be resolved to a type"

Tillerino commented 10 years ago

Everything should be available. Did you forget https://github.com/Tillerino/osuApiConnector?

ThePooN commented 10 years ago

please check this https://github.com/Tillerino/osuApiConnector/issues/1

ThePooN commented 10 years ago

TillerinoBot is now compilling with my latest commits.

ThePooN commented 10 years ago

merged your repo with mine.

Tillerino commented 10 years ago

There are compile errors in TestBackend.

lombok generates the constructor for the BeatmapMeta class using all its fields. You added three boolean is* fields, which changed the constructor. As these fields are only used in formInfoMessage, I suggest that you move them into that method.

There is another problem: you're using pointer comparison to check for string equality. mod.getShortName() == "DT" does not correctly identify the mods. Since Mods is an enum, you can however compare instances directly: mod == Mods.DoubleTime.

Third problem: you're copying a lot. That's never good.

I suggest, that instead of doing all of these calculations in BeatmapMeta, we do them in OsuApiBeatmap. Then we can just call

estimateMessage += " | " + secondsToMinuteColonSecond(getBeatmap().getTotalLength(mods));
if(mods != 0)
    estimateMessage += " ★ " + format.format(getBeatmap().getStarDifficulty());
estimateMessage += " ♫ " + format.format(getBeatmap().getBpm(mods));
estimateMessage += " AR" + format.format(getBeatmap().getApproachRate(mods));
ThePooN commented 10 years ago

done. did a PR on osuApiConnector https://github.com/Tillerino/osuApiConnector/pull/2

ThePooN commented 10 years ago

if you didn't see, i also did this commit: https://github.com/ThePooN/Tillerinobot/commit/9d298f1638989c81aa9eeb7aeaf5997576ac9ed8

Tillerino commented 10 years ago

I didn't, since there's no actual pull request :)

The AR calculations in osuApiConnector are actually wrong. They are based on an old chart in the osu wiki, which was updated two weeks ago. I was actually wondering why my factors in calcAR were different from your factors for beatmap length calculation. I had the same problem with OR (I also used the old charts on the wiki as a reference), but Tom corrected me.

Your latest commit looks perfect. Can you squash it on to the master?

ThePooN commented 10 years ago

do you mean a Pull Request?

Tillerino commented 10 years ago

I mean can you make a single commit, which is based on the master. Right now it's 8 commits :)

Oh one more thing: I made a fake Tillerinobot interface along with a fake backend: tillerino.tillerinobot.LocalConsoleTillerinobot. Have you tried it? You can see your changes right away.

ThePooN commented 10 years ago

nice, gonna test it! will squash my commits after :P

ThePooN commented 10 years ago

my modifications works perfectly. gotta squash them, and PR :)

ThePooN commented 10 years ago

i tried a lot of methods but i can't squash, because of your update that I merged in my repo :/

Tillerino commented 10 years ago

you can always soft-reset onto master and then commit

On Wed, Oct 1, 2014 at 9:00 PM, ThePooN notifications@github.com wrote:

i tried a lot of methods but i can't squash, because of your update that I merged in my repo :/

— Reply to this email directly or view it on GitHub https://github.com/Tillerino/Tillerinobot/issues/5#issuecomment-57517523 .

ThePooN commented 10 years ago

i may do this. i am busy for the moment.

Tillerino commented 10 years ago

Done: https://github.com/Tillerino/Tillerinobot/pull/8