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

Show difficulty rating with mods applied on score pages #8599

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cl8n
Copy link
Member

@cl8n cl8n commented Feb 6, 2022

random thing my friend mentioned would be nice. not sure if this fits as an include for scoretransformer, was just the easiest way to get it to that page

@@ -54,6 +54,18 @@ public static function getMode(): string
return snake_case(get_class_basename(static::class));
}

public function difficultyRating(): ?float
Copy link
Member

Choose a reason for hiding this comment

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

If this is to actually exist here, it at very least needs a better name. A score can't have a "difficulty rating", so it would need a beatmap prefix.

Optimally I don't think it should be in here at all, but will defer to the web team to ascertain whether a structure to make that happen is possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the closest thing I can think of is making it a relation called beatmapDifficulty instead.

Similarly for the transformer although considering difficulty rating is the only relevant value beatmap_difficulty_rating could be sufficient...

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the difficulty difference is only when mods are applied, then this shouldn't run extra queries if there're no mods

Copy link
Collaborator

@nanaya nanaya Feb 8, 2022

Choose a reason for hiding this comment

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

looking again, relation is quite difficult because the difficulty table uses composite primary key. Short of adding virtual column and indexing it, it's probably better being an extra select column with subquery similar to this (although with simpler query).

But then again for single request it probably doesn't matter much.

Copy link
Collaborator

@nanaya nanaya Feb 8, 2022

Choose a reason for hiding this comment

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

And if adding skipping difficulty query, then both mode and mods should be checked in case of convert maps (which would have different difficulty rating even when no other mods).

(checking beatmap model would be required in that case, otherwise the table can be checked directly)

@@ -54,6 +54,18 @@ public static function getMode(): string
return snake_case(get_class_basename(static::class));
}

public function difficultyRating(): ?float
Copy link
Collaborator

Choose a reason for hiding this comment

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

the closest thing I can think of is making it a relation called beatmapDifficulty instead.

Similarly for the transformer although considering difficulty rating is the only relevant value beatmap_difficulty_rating could be sufficient...

Comment on lines +23 to +25
if (props.score.difficulty_rating != null) {
beatmap.difficulty_rating = props.score.difficulty_rating;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be indicator that it's a modified difficulty rating instead of simply overwriting it.

@nanaya
Copy link
Collaborator

nanaya commented Feb 9, 2022

After some discussion, the attribute should be fetched through external api server instead. Similar to score rank but on its own library class (with the data either fetched through score model/transformer itself or its own json).

@peppy
Copy link
Member

peppy commented Feb 10, 2022

To elaborate, if we just want to get this in, maybe it's okay to go with it as-is rather than draw things out, but eventually we're going to want to use the lookup microservice as it will be required in the future.

@cl8n cl8n marked this pull request as draft May 21, 2024 04:12
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.

4 participants