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

Add mods and ruleset parameters to GetDifficultyAttributesAsync #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

matte-ek
Copy link
Contributor

@matte-ek matte-ek commented Aug 21, 2024

As per a discussion in a previous PR.

Adds the ability to pass mods and ruleset to the GetDifficultyAttributesAsync method and the ability to send data in the body during a POST request.

@matte-ek matte-ek marked this pull request as draft August 21, 2024 23:27
@matte-ek
Copy link
Contributor Author

Marked as draft for now as I noticed the mods will only allow the mods passed in as an integer, and the ability to pass in a string array should probably be added. The API will not accept HD or HDHR as a string.

@matte-ek
Copy link
Contributor Author

matte-ek commented Aug 23, 2024

Sorry for being slow with this, been very busy lately and haven't had any time. Anyway I initially thought of doing some easy overloading like:

public Task<DifficultyAttributes?> GetDifficultyAttributesAsync(int id, string? ruleset = null, int? mods = null) => GetDifficultyAttributesInternalAsync(id, ruleset, mods);

public Task<DifficultyAttributes?> GetDifficultyAttributesAsync(int id, string? ruleset = null, string[]? mods = null) => GetDifficultyAttributesInternalAsync(id, ruleset, mods);

private async Task<DifficultyAttributes?> GetDifficultyAttributesInternalAsync(int id, string? ruleset = null, object? mods = null)
{ ... }

However this will have problems with ambiguity if you were trying to do something like GetDifficultyAttributesAsync(<id>). I'm not sure what direction you want to go with this, but I propose either

  • Just rename the method like GetDifficultyAttributesWithModsArrayAsync to separate them
  • Ignore bitwise flags option, just keep the string[] option, would make sense in the current state since we don't have a Mods enum anyway.
  • Add a Mods enum type, which could have an extension like Mods.FromString("HDHR") and/or Mods.FromString(["HD", "HR"]) which you would use instead.

Do you have any other better ideas?

EDIT: Or we could just have another overload without mods at all, not sure why I didn't think of that initially, lack of sleep catching up with me I guess :)

@minisbett
Copy link
Owner

What is the object overload for? I'd make one overload for passing the int, one for passing the array and one for passing a mod string.

@minisbett
Copy link
Owner

Oh my bad, I just saw that its the private method. Maybe instead just have it's own implementation in each method and then in the end we'll see how we can abstract it

@matte-ek matte-ek force-pushed the add-mods-ruleset-to-difficulty-attributes branch from b962e4b to 067fa71 Compare August 24, 2024 15:56
@matte-ek matte-ek marked this pull request as ready for review August 24, 2024 16:04
@matte-ek
Copy link
Contributor Author

I'm pretty happy over how things are right now. Please let me know if you want anything to be changed.

Everything seems to work fine from my quick testing

var score = await OsuApiClient.GetDifficultyAttributesAsync(2833172); // diff-rating: 6.22964001
var scoreMania = await OsuApiClient.GetDifficultyAttributesAsync(2833172, "mania"); // diff-rating: 3.01440001
var scoreHr = await OsuApiClient.GetDifficultyAttributesAsync(2833172, ["HR"]); // diff-rating: 6.45979977
var scoreHrBitset = await OsuApiClient.GetDifficultyAttributesAsync(2833172, 16); // diff-rating: 6.45979977
var scoreDtMania = await OsuApiClient.GetDifficultyAttributesAsync(2833172, ["DT"], "mania"); // diff-rating: 4.05667019

067fa71 is somewhat outside the scope of this PR but is a bug I noticed while retrieving scores, so added it to this PR.

OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/OsuApiClient.cs Outdated Show resolved Hide resolved
@minisbett
Copy link
Owner

Just noticed that the change to use the Ruleset enum would not work because the post request creates the json via JsonContent, and not via Newtonsoft.Json. This should be change such that it uses NSJ for it, and instead a StringContent.

Could you add another overload for specifying strings such as HDDT? That would be done by doing .Chunk(2) on the string and then calling the string array overload on it.

@matte-ek
Copy link
Contributor Author

Am I being totally stupid, or has StringEnumConverter::WriteJson ever worked? JsonSerializationException will always be thrown.

@minisbett
Copy link
Owner

minisbett commented Aug 26, 2024

Am I being totally stupid, or has StringEnumConverter::WriteJson ever worked? JsonSerializationException will always be thrown.

Never tried it lol. Will have to look into that my bad, saw the commit. lol.

OsuSharp/Converters/StringEnumConverter.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/Endpoints/Beatmaps.cs Outdated Show resolved Hide resolved
OsuSharp/OsuApiClient.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants